[dri people: please have a look at the KMS discussion way below]
On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote:
[note: please configure your email client properly so it keeps
proper attribution of text and and does not rewrap the citations
incorrectly. Wrap your own text after 70 characters]
I think I need to clarify to things:
* When I talk about a bus, I mean 'struct bus_type', which identifies
all devices with a uniform bus interface to their parent device
(think: PCI, USB, I2C). You seem to think of a bus as a specific
instance of that bus type, i.e. the device that is the parent of
all the connected devices. If you have only one instance of a bus
in any system, and they are all using the same driver, do not add
a bus_type for it.
A good reason to add a bus_type would be e.g. if the "display"
driver uses interfaces to the dss that are common among multiple
dss drivers from different vendors, but the actual display drivers
are identical. This does not seem to be the case.
* When you say that the devices are static, I hope you do not mean
static in the C language sense. We used to allow devices to be
declared as "static struct" and registered using
platform_device_register (or other bus specific functions). This
is no longer valid and we are removing the existing users, do not
add new ones. When creating a platform device, use
platform_device_register_simple or platform_device_register_resndata.
I'm not sure what you mean with drivers being static. Predefining
the association between displays and drivers in per-machine files is
fine, but since this is really board specific, it would be better
to eventually do this through data passed from the boot loader, so
you don't have to have a machine file for every combination of displays
that is in the field.
This is already the order in which you submitted them, I don't see a
difference here. I was not asking to delay any of the code, just to put
them in a logical order.
Well, developer time does not appear to be one of your problems, you
already wasted tons of it by developing a huge chunk of code that isn't
going anywhere because you wrote it without consulting the upstream
community ;-)
There is no need to develop anything from scratch here, you already have
the code you want to end up with. What I would do here is to start with
a single git commit that adds the full driver. Then take out bits you
don't absolutely need to keep the driver from showing text on your
screen (not necessarily in this order):
* Take out display drivers one by one, until there is only one left.
Do a git commit after each driver
* Take out the register definitions that are not actually used in your
code
* Remove the infrastructure for dynamic displays and hardcode the one
you use
* Take out the frame buffer code
* Take out the infrastructure for multiple user-interfaces, hardcoding KMS
to the DSS
* Anything else you don't absolutely need.
Finally, you should end up with a very lean driver that only does a
single thing and only works on one very specific board. Remove that, too,
in a final commit. Now use git to reverse the patch order and you have
a nice series that you can use for patch submission, one feature at a
time. Then we can discuss the individual merits of each patch.
In the future, best plan for how you want to submit the code while
you're writing it, instead of as an afterthought. Quite often, the
first patch to submit is also one of the early stages of the driver,
so there is no need to wait for the big picture before you start
submitting. This way, we can work out conceptual mistakes early on,
saving a lot of your time, and the reviewer's time as well.
Right, you have to find a different solution for those. Most importantly,
a module in one directory should not have intimate knowledge of data
structures in a different module in another directory.
In your example, drivers/misc is probably wrong anyway. Try ignoring this
problem at first by forcing all the drivers loadable modules, which will
naturally fix the initialization order. When you still have link order
problems by building all the drivers into the kernel after this, we can
have another look to find the least ugly solution.
I mean the layering of loadable modules: you cannot make a high-level
module link against multiple low-level modules that export the
same interface. If you have multiple modules that implement the same
interface like you diplay drivers, they need to be on top!
The way you describe it, I would picture it differently:
+----------+ +----+-----+-----+ +-------+
| mcde_hw | | fb | kms | v4l | | displ |
+----+----------------------------------+
| HW | mcde_dss |
+----+----------------------------------+
In this model, the dss is the core module that everything else
links to. The hw driver talks to the actual hardware and to the
dss. The three front-ends only talk to the dss, but not to the
individual display drivers or to the hw code directly (i.e. they
don't use their exported symbols or internal data structures.
The display drivers only talk to the dss, but not to the front-ends
or the hw drivers.
Would this be a correct representation of your modules?
Ok, so you have identified a flaw with the existing KMS code. You should
most certainly not try to make your driver fit into the flawed model by
making it look like a PC. Instead, you are encouraged to fix the problems
with KMS to make sure it can also meet your requirements. The reason
why it doesn't do that today is that all the existing users are PC
hardware and we don't build infrastructure that we expect to be used
in the future but don't need yet. It would be incorrect anyway.
Can you describe the shortcomings of the KSM code? I've added the dri-devel
list to Cc, to get the attention of the right people.
Ok.
Any chance you could change the identifiers in the code for this
without confusing other people?
Ok. If your frame buffers are not children of the displays, they should
however be children of the controller:
.../mcde_controller/
/chnlA/
/displ_crtc0
/displ_dbi0
/chnlB/
dspl_crtc1
/fb0
/fb1
/fb2
/v4l_0
/v4l_1
Does this fit better?
That is fine, you don't need to do that for products. However, it
is valuable to be able to do it and to think of it in this way.
When you are able to have everything modular, it is much easier to
spot layering violations and you can much easier define the object
life time rules.
Also, for the general case of building a cross-platform kernel,
you want to be able to use modules for everything. Remember that
we are targetting a single kernel binary that can run on multiple
SoC families, potentially with hundreds of different boards.
This makes it pretty obvious that the channel should not be a
device, but rather something internal to the dss or hw module.
What is the relation between a port/connector and a display?
If it's 1:1, it should be the same device.
You don't need to instantiate the device from the board though,
just provide the data. When you add the display specific data
to the dss data, the dss can create the display devices:
static struct mcde_display_data mcde_displays[2] = {
{
...
}, {
...
},
};
static struct mcde_dss_data {
int num_displays;
struct mcde_display_data *displays;
} my_dss = {
.num_displays = 2,
.displays = &mcde_displays;
};
The mcde_dss probe function then takes the dss_data and iterates
the displays, creating a new child device for each.
Right, this also gets obsolete, since as you said an fb cannot be
the child of a display.
Not sure how the fb setup can be both board specific and dynamic.
If it's statically defined per board, it should be part of the
dss data, and dss can then create the fb devices. If it's completely
dynamic, it gets created through user space interaction anyway.
This sounds like an odd thing for a customer to ask for ;-)
In my experience customers want to solve specific problems like
everyone else, they have little interest in adding complexity
for the sake of it. Is there something wrong with one of the
interfaces? If so, it would be better to fix that than to add
an indirection to allow more of them!
I still don't understand, sorry for being slow. Why does a camera
use a display?
Arnd
--
