login
Search
[dri people: please have a look at the KMS discussion way below] [note: please configure your email client properly so it keeps proper attribution of text and and does not rewrap the citations 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 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 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 ...
Is this part of the generic ARM runtime multi-platform kernel and device trees shebang? The Ux500 still isn't in that sector, it needs extensive rewriting of arch/arm/mach-ux500 to be done first, so as to support e.g. U8500 and U5500 with a single kernel image. Trying to skin that cat that as part of this review is a bit too much to ask IMO, I'd rather have the author of this driver adapt to whatever platform data registration mechanism is in place for the merge window. Else it needs fixing as part of a bigger endavour to root out compile-time platform configuration. Yours Linus Walleij --
The 'no static devices' rule is something that Greg brought up at the embedded developer session during PlumbersConf this year, I wasn't aware of the problem before that either. It is not related to the multi-platform kernel work and it's not ARM specific. Maybe Greg can give a short explanation of the impact of this. AFAIR, static device definitions still work, but there are plans to remove that capability in the future. Arnd --
That is exactly correct. A struct device is a dynamically referenced thing, and as such, should be dynamically created and it will be automatically destroyed when it needs to when everyone is finished with it. By making a struct device static, that kind of defeats the whole purpose of reference counting the thing, not to mention making freeing the object when finished a bit difficult :) thanks, greg k-h --
There's lots of static devices, not only platform devices, in the ARM tree. It's going to be a hell of a lot of work to fix this all up properly. I hope that the capability for static devices won't disappear until the huge pile of work on ARM has been completed. --
Don't worry, it will not. thanks, greg k-h --
I don't agree that it is abuse - it was something explicitly allowed by the original device model design by Patrick, with the condition that such a device was never unregistered. That's exactly the way we treat these devices. What I'm slightly concerned about is that this is going to needlessly bloat the kernel - we're going to have to find some other way to store this information, and create devices from that - which means additional code to do the creation, and data structures for it to create these from. There will be additional wastage from kmalloc as kmalloc doesn't allocate just the size you ask for, but normally a power of two which will contain the size. That could potentially mean that as the device structure is 216 bytes, kmalloc will use the 256 byte allocation size, which means a wastage of 40 bytes per device structure. On top of that goes the size of resources with the allocation slop on top for that, and then there's another allocation for the platform data. Has anyone considered these implications before making this choice? --
I understand Pat allowed this, I just don't agree that it's the correct thing to do :) -mm had a patch for a long time that would throw up warnings if you ever Yes, I have, which is one reason I haven't done this type of change yet. I need to figure out a way to not drasticly increase the size and still make it easy and simple for the platform and driver write their code. It's a work in progress, but wherever possible, I encourage people to not make 'struct device' static. thanks, greg k-h --
Right, so saying to ARM developers that they can't submit code which adds new static device structures is rather problematical then, and effectively brings a section of kernel development to a complete standstill - it means no support for additional ARM platforms until this issue is resolved. (This "condition" was mentioned by Arnd earlier in this thread, and was put in such a way that it was now a hard and fast rule.) I feel it would be better to allow the current situation to continue. If we start telling people that they can't use statically declared devices without first having an alternative, we'll end up with people inventing their own individual - and different - solutions to this problem, which could actually make the problem harder to resolve in the longer term. --
Sorry, I didn't mean for that to be mentioned that way at all, as I know Ok, but again, I do encourage, wherever possible, that people do not statically create a 'struct device'. thanks, greg k-h --
I think this misses the point, and is somewhat redundant; I think everyone knows that it is easiest to never change anything. But I would consider it early warning that the way things have been done before will go away. And I would thus write drivers in a way that demonstrates and includes that understanding. The same problem exists in hardware products needing any kind of longish lifetime. Deal with evolving components by having clean and simple interfaces, and by not relying on a particular interface very deep on either side of the edge. Simple I think. //Peter --
Clearly you haven't understood my point. If we go down the route you suggest, we're going to end up with everyone doing something different, which will then need to be cleaned up once the proper solution is in place. That could easily become much more work than just waiting for the proper solution. What is easier - to fix all instances which statically declare, or to fix all instances which statically declare _and_ all the bodged up alternative solutions? --
At the embedded developer BoF in Cambridge,MA we discussed this problem quite a bit, and my impression there was that it is a hard Yes, that makes sense. We should probably start thinking about the migration plan though. There does not seem to be a shortage of alternatives for registering new platform devices already and once we can get to agree on how we want it done, we can start mandating that new drivers do it that way, while we phase out some of the other ones. Among the architectures that use platform devices extensively, the various ways to register them I could find are: * static platform_register_device: arm, avr32, frv, mips, m32r, sparc, sh and xtensa * static platform_add_devices: arm, blackfin, m68knommu, mips, sh * dynamic platform_device_register_simple: m68k, mips, powerpc, sh and x86 * dynamic platform_device_alloc/platform_device_add: arm, avr32, mips, powerpc, lots of drivers * dynamic of_platform_bus_probe: powerpc, microblaze * dynamic platform_device_register_resndata: not currently used I was under the impression that platform_device_register_resndata is the function that was actually meant to solve this, but there are exactly zero users of this, except for the platform_device_register_simple wrapper. The fact that it's currently not used probably means either that nobody heard about it or that the interface is lacking in some way and is actually useless for replacing the static definitions. Arnd --
I'm not sure I've a full understanding of what this bus is all about, but I can't see why it can't fit inside KMS, with maybe a V4L bolted on. The whole point of KMS is to provide a consistent userspace interface for describing the graphics hardware in enough detail that userspace can use it, but without giving it all the gorey details. So we've reduced the interface to crtc/encoder/connectors as the base level objects at the interface, internally drivers can and do have extra layers, but usually no need to show this to userspace. KMS at the moment doesn't really handle dynamic hotplug of new crtcs connectors etc, but I'm not sure that is needed here. It sounds like you just have some embedded building blocks you want to put together on a design by design basis, please correct me if I'm wrong. --
This doesn't seem that different from the graphics chips we support with kms. I don't think it would require much work to use KMS. One thing we considered, but never ended up implementing was a generic overlay API for KMS. Most PC hardware still has overlays, but we don't use them much any more on the desktop side. It may be worthwhile to design an appropriate API for them for these type of hardware. To elaborate on the current KMS design, we have: crtcs - the display controller. these map to the timing generators and scanout hardware encoders - the hw that takes the bitstream from the display controller and converts it to the appropriate format for the connected display. connector - the physical interface that a display attaches to (VGA, LVDS, eDP, HDMI-A, etc.) Modern PC hardware is pretty complex. I've blogged about some of the recent radeon display hardware: http://www.botchco.com/agd5f/?p=51 Moreover, each oem designs different boards with vastly different display configurations. It gets more complex with things like advanced color management and DP (DisplayPort) 1.2 that introduces things like daisy-chaining monitors and tunnelling USB and audio over --
Just fyi about a generic overlay api: I've looked a bit into this when doing the intel overlay support and I think adding special overlay crtcs that can be attached real crtcs gives a nice clean api. We could the reuse the existing framebuffer/pageflipping api to get the buffers to the overlay engine. The real pain starts when we want format discovery from userspace with all the alignement/size/layout constrains. Add in tiling support and its almost impossible to do in a generic way. But also for kms userspace needs to know these constrains (implemented for generic use in libkms). I favor such an approach for overlays, too (if it ever happens) - i.e. a few helpers in libkms that allocate an appropriate buffer for a given format and size and returns the buffer, strides and offsets for the different planes. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 --
I'm now using Thunderbird, please let me know if it's better than my Correct, I refer to the device, not type or driver. I used a bus type since it allowed me to setup a default implementation for each driver callback. So all drivers get generic implementation by default, and override when that is not enough. Meybe you have a better way of getting I guess you have read the ARM vs static platform_devices. But, yes, I mean in the c-language static sense. I will adopt to whatever Russel We are now taking a step back and start "all over". We were almost as fresh on this HW block as you are now when we started implementing the driver earlier this year. I think all of us benefit from now having a better understanding of customer requirements and the HW itself, there are some nice quirks ;). Anyway, we will restart the patches and RFC only the MCDE HW part of the driver, implementing basic fb support for one display board as you suggested initially. It's a nice step towards making the patches easier to review and give us some time to prepare the DSS stuff. That remake was done today, so I think the patch will be sent Hope not, we have learned a lot, and we are now ready for a second refactoring of the driver. Now that most of the features needed are in place. Allowing us also to remove any driver code/feature that was never This is how we will try to work now that we know how the HW works. I you Relying on per folder load order might solve most of the ordering I don't think we do. The layers are very strict. If you found some code Hmm, mcde_hw does not link to dss. It should be FB->DSS->Display driver->MCDE_HW->HW IO (+ DSS->MCDE_HW). My picture is how code should I will start this work early next year. MCDE DSS refactoring will take KMS into account. Some of the _possible_ short comings (I must say I have not looked into this in any detail yet): - 3D HW is bundled with display HW. Makes it harder for us to use different 3D HW with same display HW or the other ...
Much better now, just remember to leave empty lines around your replies One solution that I like is to write a module with the common code as a library, exporting all the default actions. The specific drivers can then fill their operations structure by listing the defaults or by providing their own functions to replace them, which in turn Fair enough. We will have to fix it some day so Greg can go on with his plan to disallow static devices, but for now I'm not going to stop you. I would use platform_device_register_simple anyway, but feel Ok, sounds great! I'm also starting a 3 week vacation, but will be at the Linaro sprint in Dallas. My feeling now, after understanding about it some more, is that it would actually be better to start with a KMS implementation instead of a classic frame buffer. Ideally you wouldn't even need the frame buffer driver or the multiplexing between the two then, but still get all the benefits I don't think it makes any sense to have the DSS sit on top of the display drivers, since that means it has to know about all of them and loading the DSS module would implicitly have to load all the display modules below it, even for the displays that are not present. Moreover, I don't yet see the reason for the split between mcde_hw and dss. If dss is the only user of the hardware module (aside from stuff using dss), and dss is written against the hw module as a low-level Ok. I'd have to look into this in more detail myself to see how severe this is, or how to solve it. The problem seems obvious This will be a lot tougher for you. External modules are generally not accepted as a reason for designing code one way vs. another. Whatever the solution is, you have to convince people that it would still make sense if all drivers were part of the kernel itself. Bonus points to you if you define it in a way that forces the 3d driver I have no idea what this means, but I trust that you and others Remember that with ioctls, you can always add new ...
I will look at it, we might still post a fb->mcde_hw first though, since DSS does not have a static dependency on display drivers. DSS is just a "convenience library" for handling the correct display driver call sequences, instead of each user (fbdev/KMS/V4L2) having to duplicate I see this as a side effect of DRM putting a dependency between 3D HW and KMS HW driver. In most embedded systems, these two are no more coupled than any other HW block on the SoC. So by "fixing" this _possible_ flaw. I see no reason why a KMS driver can't stand on it's own. There's no reason not to support display in the kernel just because Sure, but that is currently up to board init code. Just as for frame buffers, mcde_fb_create(display, ...), we will have a "createV4L2device(display, ...)". /BR /Marcus --
