Re: [PATCH 09/10] MCDE: Add build files and bus
On Mon, Mar 14, 2011 at 9:03 AM, Marcus Lorentzon marcus.xm.lorent...@stericsson.com wrote: On 03/12/2011 04:59 PM, Rob Clark wrote: On Sun, Dec 5, 2010 at 5:28 AM, Daniel Vetterdan...@ffwll.ch wrote: On Sat, Dec 04, 2010 at 04:34:22PM -0500, Alex Deucher wrote: 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. 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. btw, has there been any further thought/discussion on this topic.. I've been experimenting with a drm driver interface on the OMAP SoC. It is working well now for framebuffer type usage (mode setting, virtual framebuffer spanning multiple diplays, and those types of xrandr things).. the next step that I've started thinking about is overlay (or underlay.. the z-order is flexible) support.. I was thinking in a similar direction (ie. a special, or maybe not so special, sort of crtc) and came across this thread, so I thought I'd resurrect the topic. In our case, most of the CRTCs in our driver could be used either with (A)RGB buffers as a traditional framebuffer, or with a few different formats of YUV as video under/overlays. So if you had one display attached, you might only use one CRTC for traditional GUI/gfx layer, and the rest are available for video. If you had two displays, then you'd steal one of the video CRTCs and use it for the gfx layer on the second display. And so on. We have similar HW and are also interested in finding some common ground for overlays in KMS. Just as you describe, we have no hard connection between a CRTC and output. Instead we only have overlays. Normal gfx use case is then of course one of these overlays dedicated to one display. And when adding video overlays, we also prefer YUV underlays with fullscreen ARGB gfx on top. The problem with mapping this to the CRTCs in KMS today, is that there is no differentiation between framebuffer width/height and crt width/height. And of course YUV formats and fb position etc are missing. One advantage of the set CRTC ioctl is that all information needed to switch mode is contained in one atomic set mode ioctl. So we have to think about if we want a new more advanced set crtc including overlay config. Or if we want to split mode setup into several requests. And then we must decide if multiple setup ioctls will need some type of commit to get the atomic mode switch we have today. For example I don't want to have to do a set_crtc enabling blending without overlay being setup. It should be just as glitch free as KMS is today. Rough thinking: + add some 'caps' to the CRTC to indicate whether it can handle YUV, ARGB, scaling + add an x/y offset relative to the encoder (as opposed to the existing x/y offset relative to the framebuffer) + add a z-order parameter Exactly what I would like to have. Especially the caps for scaling, since we have one HW that can't do scaling. Not sure about intel hw if it is supporting clip-rects.. if so, maybe need to add something about that. In our case we jut put the video behind the gfx layer and use the alpha channel in the gfx framebuffer to clip/blend rather than using clip-rects. If this is common ground, I would like to have one clip rect per CRTC/overlay to enable framebuffers larger than overlay viewport. That makes it easier to reuse a large buffer for multiple overlays/framebuffers without having to stress memory management driver. But this is just a nice to have feature. Maybe this can be mapped to stride/start address mappings on HW without clip rect. But that will probably include alignment requirements on position and size. Good point, I had overlooked that but we do have same requirement for cropping as well.. although in the crtc we already specify an x/y offset within the drm_framebuffer that the crtc is attached to.. so I guess if we have an input width/height (output is implied I guess by the encoder/connector) then we should be fine for cropping Although in some cases top/left crop offset could be changing frame by frame (think use cases like zero-copy video stabilization or pan/scan) so might be nice to have a way to specify new x/y offset when flipping. I guess that would be an extension/change to existing page flip ioctl. BR, -R The real pain starts when we want format discovery from userspace with all the alignement/size/layout
Re: [PATCH 09/10] MCDE: Add build files and bus
On 12/17/2010 12:22 PM, Arnd Bergmann wrote: * 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. 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 the same behavior. 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 can call the default functions. This is e.g. what libata does. Will do. 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 out soon. (I'm going on vacation for 3 weeks btw). 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 from the new KMS infrastructure. I will look at it, we might still post a fb-mcde_hw first though, since it's so little work. DSS give access to all display devices probed on the virtual mcde dss bus, or platform bus with specific type of devices if you like. All calls to DSS operate on a display device, like create an overlay(=framebuffer), request an update, set power mode, etc. All calls to DSS related to display itself and not only framebuffer scanout, will be passed on to the display driver of the display device in question. All calls DSS only related to overlays, like buffer pointers, position, rotation etc is handled directly by DSS calling mcde_hw. You could see mcde_hw as a physical level driver and mcde_dss closer to a logical driver, delegating display specific decisions to the display driver. Another analogy is mcde_hw is host driver and display drivers are client device drivers. And DSS is a collection of logic to manage the interaction between host and client devices. 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? 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 be used. Anything else you find in code is a violation of that layering. 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. 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 this code. Moreover, I don't yet see the
Re: [PATCH 09/10] MCDE: Add build files and bus
On 11/26/2010 12:24 PM, Arnd Bergmann wrote: [dri people: please have a look at the KMS discussion way below] On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote: -Original Message- From: Arnd Bergmann [mailto:a...@arndb.de] Sent: den 25 november 2010 17:48 To: Marcus LORENTZON Cc: linux-arm-ker...@lists.infradead.org; Jimmy RUBIN; linux- me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ Subject: Re: [PATCH 09/10] MCDE: Add build files and bus On Thursday 25 November 2010, Marcus LORENTZON wrote: From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday 10 November 2010, Jimmy Rubin wrote: This patch adds support for the MCDE, Memory-to-display controller, found in the ST-Ericsson ux500 products. [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'm now using Thunderbird, please let me know if it's better than my previous webmail client, neither have many features for reply formatting. All devices that you cannot probe by asking hardware or firmware are normally considered platform devices. Then again, a platform device is usally identified by its resources, i.e. MMIO addresses and interrupts, which I guess your display does not have. Then we might be on right track to model them as devices on a platform bus. Since most displays/panels can't be plug-n-play probed, instead devices has to be statically declared in board-xx.c files in mach-ux500 folder. Or is platform bus a singleton? Or can we define a new platform bus device? Displays like HDMI TV-sets are not considered for device/driver in mcde. Instead there will be a hdmi-chip-device/driver on the mcde bus. So all devices and drivers on this bus are static. 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. 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 the same behavior. * 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. 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 King says is The right way in ARM SoCs. Staging it in a way that adds all the display drivers later than the base driver is a good idea, but it would be helpful to also add the infrastructure at the later stage. Maybe you can try to simplify the code for now by hardcoding the single display and remove the dynamic registration. You still have the the code, so once the base code looks good for inclusion, we can talk about it in the context of adding dynamic display support back in, possibly in exactly the way you are proposing now, but perhaps in an entirely different way if we come up with a better solution. What about starting with MCDE HW, which is the core HW driver doing all real work? And then continue with the infrastructure + some displays + drivers ... 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. 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
Re: [PATCH 09/10] MCDE: Add build files and bus
On Sat, Dec 04, 2010 at 04:34:22PM -0500, Alex Deucher wrote: 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. 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: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Fri, Nov 26, 2010 at 6:24 AM, Arnd Bergmann a...@arndb.de wrote: [dri people: please have a look at the KMS discussion way below] On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote: -Original Message- From: Arnd Bergmann [mailto:a...@arndb.de] Sent: den 25 november 2010 17:48 To: Marcus LORENTZON Cc: linux-arm-ker...@lists.infradead.org; Jimmy RUBIN; linux- me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ Subject: Re: [PATCH 09/10] MCDE: Add build files and bus On Thursday 25 November 2010, Marcus LORENTZON wrote: From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday 10 November 2010, Jimmy Rubin wrote: This patch adds support for the MCDE, Memory-to-display controller, found in the ST-Ericsson ux500 products. [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] All devices that you cannot probe by asking hardware or firmware are normally considered platform devices. Then again, a platform device is usally identified by its resources, i.e. MMIO addresses and interrupts, which I guess your display does not have. Then we might be on right track to model them as devices on a platform bus. Since most displays/panels can't be plug-n-play probed, instead devices has to be statically declared in board-xx.c files in mach-ux500 folder. Or is platform bus a singleton? Or can we define a new platform bus device? Displays like HDMI TV-sets are not considered for device/driver in mcde. Instead there will be a hdmi-chip-device/driver on the mcde bus. So all devices and drivers on this bus are static. 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. Staging it in a way that adds all the display drivers later than the base driver is a good idea, but it would be helpful to also add the infrastructure at the later stage. Maybe you can try to simplify the code for now by hardcoding the single display and remove the dynamic registration. You still have the the code, so once the base code looks good for inclusion, we can talk about it in the context of adding dynamic display support back in, possibly in exactly the way you are proposing now, but perhaps in an entirely different way if we come up with a better solution. What about starting with MCDE HW, which is the core HW driver doing all real work? And then continue with the infrastructure + some displays + drivers ... 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. Only problem is that we then have a driver that can't be used from user space, because I don't think I can find anyone with enough time to write a display driver + framebuffer on top of mcde_hw (which is what the existing code does). 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
Re: [PATCH 09/10] MCDE: Add build files and bus
Russell King - ARM Linux wrote: I feel it would be better to allow the current situation to continue. I think this misses the point, and is somewhat redundant; I think everyone knows that it is easiest to never change anything. But then nothing improves. If we start telling people that they can't use statically declared devices without first having an alternative, 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Wed, Dec 01, 2010 at 01:53:39PM +0100, Peter Stuge wrote: Russell King - ARM Linux wrote: I feel it would be better to allow the current situation to continue. I think this misses the point, and is somewhat redundant; I think everyone knows that it is easiest to never change anything. But then nothing improves. If we start telling people that they can't use statically declared devices without first having an alternative, 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. 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? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Wednesday 01 December 2010, Russell King - ARM Linux wrote: 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.) 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 rule indeed, so I said this to the best of my knowledge. 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
2010/11/26 Arnd Bergmann a...@arndb.de: * 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tuesday 30 November 2010, Linus Walleij wrote: 2010/11/26 Arnd Bergmann a...@arndb.de: * 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. 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 04:21:47PM +0100, Arnd Bergmann wrote: On Tuesday 30 November 2010, Linus Walleij wrote: 2010/11/26 Arnd Bergmann a...@arndb.de: * 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. 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. 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 04:21:47PM +0100, Arnd Bergmann wrote: On Tuesday 30 November 2010, Linus Walleij wrote: 2010/11/26 Arnd Bergmann a...@arndb.de: * 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. 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. 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. 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. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 06:40:49PM +, Russell King - ARM Linux wrote: On Tue, Nov 30, 2010 at 04:21:47PM +0100, Arnd Bergmann wrote: On Tuesday 30 November 2010, Linus Walleij wrote: 2010/11/26 Arnd Bergmann a...@arndb.de: * 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. 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. 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. 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 agree, it's been abused for many years this way :( 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote: On Tue, Nov 30, 2010 at 06:40:49PM +, Russell King - ARM Linux wrote: 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 agree, it's been abused for many years this way :( 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? -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 10:05:50PM +, Russell King - ARM Linux wrote: On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote: On Tue, Nov 30, 2010 at 06:40:49PM +, Russell King - ARM Linux wrote: 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 agree, it's been abused for many years this way :( 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. 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 did this for x86 so that arch should be clean of this issue by now. 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? 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 03:05:33PM -0800, Greg KH wrote: On Tue, Nov 30, 2010 at 10:05:50PM +, Russell King - ARM Linux wrote: On Tue, Nov 30, 2010 at 10:48:34AM -0800, Greg KH wrote: On Tue, Nov 30, 2010 at 06:40:49PM +, Russell King - ARM Linux wrote: 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 agree, it's been abused for many years this way :( 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. 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 did this for x86 so that arch should be clean of this issue by now. 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? 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. 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. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
On Tue, Nov 30, 2010 at 11:42:15PM +, Russell King - ARM Linux wrote: It's a work in progress, but wherever possible, I encourage people to not make 'struct device' static. 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.) Sorry, I didn't mean for that to be mentioned that way at all, as I know the issues that are keeping this from happening. 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. Ok, but again, I do encourage, wherever possible, that people do not statically create a 'struct device'. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/10] MCDE: Add build files and bus
[dri people: please have a look at the KMS discussion way below] On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote: -Original Message- From: Arnd Bergmann [mailto:a...@arndb.de] Sent: den 25 november 2010 17:48 To: Marcus LORENTZON Cc: linux-arm-ker...@lists.infradead.org; Jimmy RUBIN; linux- me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ Subject: Re: [PATCH 09/10] MCDE: Add build files and bus On Thursday 25 November 2010, Marcus LORENTZON wrote: From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday 10 November 2010, Jimmy Rubin wrote: This patch adds support for the MCDE, Memory-to-display controller, found in the ST-Ericsson ux500 products. [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] All devices that you cannot probe by asking hardware or firmware are normally considered platform devices. Then again, a platform device is usally identified by its resources, i.e. MMIO addresses and interrupts, which I guess your display does not have. Then we might be on right track to model them as devices on a platform bus. Since most displays/panels can't be plug-n-play probed, instead devices has to be statically declared in board-xx.c files in mach-ux500 folder. Or is platform bus a singleton? Or can we define a new platform bus device? Displays like HDMI TV-sets are not considered for device/driver in mcde. Instead there will be a hdmi-chip-device/driver on the mcde bus. So all devices and drivers on this bus are static. 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. Staging it in a way that adds all the display drivers later than the base driver is a good idea, but it would be helpful to also add the infrastructure at the later stage. Maybe you can try to simplify the code for now by hardcoding the single display and remove the dynamic registration. You still have the the code, so once the base code looks good for inclusion, we can talk about it in the context of adding dynamic display support back in, possibly in exactly the way you are proposing now, but perhaps in an entirely different way if we come up with a better solution. What about starting with MCDE HW, which is the core HW driver doing all real work? And then continue with the infrastructure + some displays + drivers ... 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. Only problem is that we then have a driver that can't be used from user space, because I don't think I can find anyone with enough time to write a display driver + framebuffer on top of mcde_hw (which is what the existing code does). 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
RE: [PATCH 09/10] MCDE: Add build files and bus
Hi Arnd,
Comments inline.
-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: den 12 november 2010 17:23
To: linux-arm-ker...@lists.infradead.org
Cc: Jimmy RUBIN; linux-fb...@vger.kernel.org; linux-
me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
On Wednesday 10 November 2010, Jimmy Rubin wrote:
This patch adds support for the MCDE, Memory-to-display controller,
found in the ST-Ericsson ux500 products.
This patch adds the necessary build files for MCDE and the bus that
all displays are connected to.
Can you explain why you need a bus for this?
The idea of the bus is to make it easier to add new panel/display support using
the normal device/driver model. Boards add devices at init, and drivers are
selected in config. If we were to model the real physical bus structure it
would be very complex, hard to use. We use our own bus, but it's really a
virtual bus for adding display devices and drivers to MCDE host. Is there any
generic virtual bus type we should use instead of our own type? If you have
another idea of how to add display devices and drivers without MCDE code
modifications, please let us know. A virtual bus just give us a nice framework
to do this.
With the code you currently have, there is only a single driver
associated
with this bus type, and also just a single device that gets registered
here!
And yes, currently there's only one single driver. But there's a lot more
coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And once all
different u8500 boards are pushed, there will be multiple boards registering
devices with different setups. But one thing at a time.
+static int __init mcde_subsystem_init(void)
+{
+ int ret;
+ pr_info(MCDE subsystem init begin\n);
+
+ /* MCDE module init sequence */
+ ret = mcde_init();
+ if (ret)
+ return ret;
+ ret = mcde_display_init();
+ if (ret)
+ goto mcde_display_failed;
+ ret = mcde_dss_init();
+ if (ret)
+ goto mcde_dss_failed;
+ ret = mcde_fb_init();
+ if (ret)
+ goto mcde_fb_failed;
+ pr_info(MCDE subsystem init done\n);
+
+ return 0;
+mcde_fb_failed:
+ mcde_dss_exit();
+mcde_dss_failed:
+ mcde_display_exit();
+mcde_display_failed:
+ mcde_exit();
+ return ret;
+}
Splitting up the module into four sub-modules and then initializing
everything from one place indicates that something is done wrong
on a global scale.
If you indeed need a bus, that should be a separate module that gets
loaded first and then has the other modules build on top of.
Yes, that's the general idea. We don't completely understand the correct way of
making sure how one module gets loaded before another, without selecting init
level, like the fs_initcall below you commented on. I guess most of our
submodules should be initialized during subsys_init. But we have not found how
to specify the module init order inside subsys init level. Maybe you can shed
some light on this? Makefile order seems like a fragile way of defining init
order dependencies.
Do you think static init calls from machine subsys init are a better solution?
I'm not sure how the other parts layer on top of one another, can you
provide some more insight?
++
| mcde_fb/mcde_kms/mcde_v4l2 |
+---++
|mcde_dss |
+ +---+
| | disp drvs |
+---+---+
|mcde hw|
+---+
| HW |
+---+
From what I understood so far, you have a single multi-channel display
controller (mcde_hw.c) that drives the hardware.
Each controller can have multiple frame buffers attached to it, which
in turn can have multiple displays attached to each of them, but your
current configuration only has one of each, right?
Correct, channels A/B (crtcs) can have two blended framebuffers plus
background color, channels C0/C1 can have one framebuffer.
Right now you have a single top-level bus device for the displays,
maybe that can be integrated into the controller so the displays are
properly rooted below the hardware that drives them.
Not sure I understand 100%. Routing is independent of bus structure. Routing
could change dynamically during runtime reconfiguration using future KMS for
example. Bus is only for connecting display devices with drivers. Of course we
could have one bus per port/connector. But then the code would be more complex
and we would end up with one bus/device/driver per connector (or in some rare
cases 2-4 on DBI/DSI connectors).
The frame buffer device also looks weird. Right now you only seem
to have a single frame buffer registered to a driver in the same
module. Is that frame buffer not dependent on a controller?
MCDE framebuffers are only depending on MCDE DSS. DSS
Re: [PATCH 09/10] MCDE: Add build files and bus
On Thursday 25 November 2010, Marcus LORENTZON wrote:
From: Arnd Bergmann [mailto:a...@arndb.de]
On Wednesday 10 November 2010, Jimmy Rubin wrote:
This patch adds support for the MCDE, Memory-to-display controller,
found in the ST-Ericsson ux500 products.
This patch adds the necessary build files for MCDE and the bus that
all displays are connected to.
Can you explain why you need a bus for this?
The idea of the bus is to make it easier to add new panel/display support
using the normal device/driver model. Boards add devices at init, and
drivers are selected in config. If we were to model the real physical
bus structure it would be very complex, hard to use. We use our own bus,
but it's really a virtual bus for adding display devices and drivers to
MCDE host. Is there any generic virtual bus type we should use instead
of our own type? If you have another idea of how to add display devices
and drivers without MCDE code modifications, please let us know. A virtual
bus just give us a nice framework to do this.
All devices that you cannot probe by asking hardware or firmware are normally
considered platform devices. Then again, a platform device is usally
identified by its resources, i.e. MMIO addresses and interrupts, which
I guess your display does not have.
With the code you currently have, there is only a single driver
associated
with this bus type, and also just a single device that gets registered
here!
And yes, currently there's only one single driver. But there's a lot more
coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And once
all different u8500 boards are pushed, there will be multiple boards
registering devices with different setups. But one thing at a time.
Your approach is making it hard to review the code in context. Adding a
device driver that uses existing infrastructure is usually straightforward,
but adding infrastructure is a hard thing and needs to be done with great
care, especially if you add infrastructure before it actually is needed.
Staging it in a way that adds all the display drivers later than the
base driver is a good idea, but it would be helpful to also add the
infrastructure at the later stage. Maybe you can try to simplify the
code for now by hardcoding the single display and remove the dynamic
registration. You still have the the code, so once the base code looks
good for inclusion, we can talk about it in the context of adding
dynamic display support back in, possibly in exactly the way you are
proposing now, but perhaps in an entirely different way if we come up
with a better solution.
On a more abstract level, when you want to add large chunks of code
to the kernel, you often cannot find anyone to review and understand
all of it at once. Splitting a subsystem into ten patches by file
level rarely helps, so instead you should ideally have a series of
patches that each add working code that enable more features.
This is of course more work for you, especially if you did not consider
it while writing the code in the first place. Still, you should
always try hard to make code easy to review as much as possible,
because you need to work with reviewers both to get code in and to
make sure you make the quality ends up as good as possible.
+static int __init mcde_subsystem_init(void)
+{
+ int ret;
+ pr_info(MCDE subsystem init begin\n);
+
+ /* MCDE module init sequence */
+ ret = mcde_init();
+ if (ret)
+ return ret;
+ ret = mcde_display_init();
+ if (ret)
+ goto mcde_display_failed;
+ ret = mcde_dss_init();
+ if (ret)
+ goto mcde_dss_failed;
+ ret = mcde_fb_init();
+ if (ret)
+ goto mcde_fb_failed;
+ pr_info(MCDE subsystem init done\n);
+
+ return 0;
+mcde_fb_failed:
+ mcde_dss_exit();
+mcde_dss_failed:
+ mcde_display_exit();
+mcde_display_failed:
+ mcde_exit();
+ return ret;
+}
Splitting up the module into four sub-modules and then initializing
everything from one place indicates that something is done wrong
on a global scale.
If you indeed need a bus, that should be a separate module that gets
loaded first and then has the other modules build on top of.
Yes, that's the general idea. We don't completely understand the correct
way of making sure how one module gets loaded before another, without
selecting init level, like the fs_initcall below you commented on. I
guess most of our submodules should be initialized during subsys_init.
But we have not found how to specify the module init order inside subsys
init level. Maybe you can shed some light on this? Makefile order seems
like a fragile way of defining init order dependencies.
Do you think static init calls from machine subsys init are a better solution?
In general, the idea with loadable modules is that you
RE: [PATCH 09/10] MCDE: Add build files and bus
-Original Message-
From: Arnd Bergmann [mailto:a...@arndb.de]
Sent: den 25 november 2010 17:48
To: Marcus LORENTZON
Cc: linux-arm-ker...@lists.infradead.org; Jimmy RUBIN; linux-
me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ
Subject: Re: [PATCH 09/10] MCDE: Add build files and bus
On Thursday 25 November 2010, Marcus LORENTZON wrote:
From: Arnd Bergmann [mailto:a...@arndb.de]
On Wednesday 10 November 2010, Jimmy Rubin wrote:
This patch adds support for the MCDE, Memory-to-display
controller,
found in the ST-Ericsson ux500 products.
This patch adds the necessary build files for MCDE and the bus
that
all displays are connected to.
Can you explain why you need a bus for this?
The idea of the bus is to make it easier to add new panel/display
support
using the normal device/driver model. Boards add devices at init, and
drivers are selected in config. If we were to model the real
physical
bus structure it would be very complex, hard to use. We use our own
bus,
but it's really a virtual bus for adding display devices and drivers
to
MCDE host. Is there any generic virtual bus type we should use
instead
of our own type? If you have another idea of how to add display
devices
and drivers without MCDE code modifications, please let us know. A
virtual
bus just give us a nice framework to do this.
All devices that you cannot probe by asking hardware or firmware are
normally
considered platform devices. Then again, a platform device is usally
identified by its resources, i.e. MMIO addresses and interrupts, which
I guess your display does not have.
Then we might be on right track to model them as devices on a platform bus.
Since most displays/panels can't be plug-n-play probed, instead devices has
to be statically declared in board-xx.c files in mach-ux500 folder. Or is
platform bus a singleton? Or can we define a new platform bus device?
Displays like HDMI TV-sets are not considered for device/driver in mcde.
Instead there will be a hdmi-chip-device/driver on the mcde bus. So all devices
and drivers on this bus are static.
With the code you currently have, there is only a single driver
associated
with this bus type, and also just a single device that gets
registered
here!
And yes, currently there's only one single driver. But there's a lot
more
coming in the pipe. Like some LCD, DPI, DBI, DSI display drivers. And
once
all different u8500 boards are pushed, there will be multiple boards
registering devices with different setups. But one thing at a time.
Your approach is making it hard to review the code in context. Adding a
device driver that uses existing infrastructure is usually
straightforward,
but adding infrastructure is a hard thing and needs to be done with
great
care, especially if you add infrastructure before it actually is
needed.
Staging it in a way that adds all the display drivers later than the
base driver is a good idea, but it would be helpful to also add the
infrastructure at the later stage. Maybe you can try to simplify the
code for now by hardcoding the single display and remove the dynamic
registration. You still have the the code, so once the base code looks
good for inclusion, we can talk about it in the context of adding
dynamic display support back in, possibly in exactly the way you are
proposing now, but perhaps in an entirely different way if we come up
with a better solution.
What about starting with MCDE HW, which is the core HW driver doing all real
work? And then continue with the infrastructure + some displays + drivers ...
Only problem is that we then have a driver that can't be used from user space,
because I don't think I can find anyone with enough time to write a display
driver + framebuffer on top of mcde_hw (which is what the existing code does).
On a more abstract level, when you want to add large chunks of code
to the kernel, you often cannot find anyone to review and understand
all of it at once. Splitting a subsystem into ten patches by file
level rarely helps, so instead you should ideally have a series of
patches that each add working code that enable more features.
This is of course more work for you, especially if you did not consider
it while writing the code in the first place. Still, you should
always try hard to make code easy to review as much as possible,
because you need to work with reviewers both to get code in and to
make sure you make the quality ends up as good as possible.
+static int __init mcde_subsystem_init(void)
+{
+ int ret;
+ pr_info(MCDE subsystem init begin\n);
+
+ /* MCDE module init sequence */
+ ret = mcde_init();
+ if (ret)
+ return ret;
+ ret = mcde_display_init();
+ if (ret)
+ goto mcde_display_failed;
+ ret = mcde_dss_init();
+ if (ret)
+ goto
Re: [PATCH 09/10] MCDE: Add build files and bus
On Wednesday 10 November 2010, Jimmy Rubin wrote:
This patch adds support for the MCDE, Memory-to-display controller,
found in the ST-Ericsson ux500 products.
This patch adds the necessary build files for MCDE and the bus that
all displays are connected to.
Can you explain why you need a bus for this?
With the code you currently have, there is only a single driver associated
with this bus type, and also just a single device that gets registered here!
+static int __init mcde_subsystem_init(void)
+{
+ int ret;
+ pr_info(MCDE subsystem init begin\n);
+
+ /* MCDE module init sequence */
+ ret = mcde_init();
+ if (ret)
+ return ret;
+ ret = mcde_display_init();
+ if (ret)
+ goto mcde_display_failed;
+ ret = mcde_dss_init();
+ if (ret)
+ goto mcde_dss_failed;
+ ret = mcde_fb_init();
+ if (ret)
+ goto mcde_fb_failed;
+ pr_info(MCDE subsystem init done\n);
+
+ return 0;
+mcde_fb_failed:
+ mcde_dss_exit();
+mcde_dss_failed:
+ mcde_display_exit();
+mcde_display_failed:
+ mcde_exit();
+ return ret;
+}
Splitting up the module into four sub-modules and then initializing
everything from one place indicates that something is done wrong
on a global scale.
If you indeed need a bus, that should be a separate module that gets
loaded first and then has the other modules build on top of.
I'm not sure how the other parts layer on top of one another, can you
provide some more insight?
From what I understood so far, you have a single multi-channel display
controller (mcde_hw.c) that drives the hardware.
Each controller can have multiple frame buffers attached to it, which
in turn can have multiple displays attached to each of them, but your
current configuration only has one of each, right?
Right now you have a single top-level bus device for the displays,
maybe that can be integrated into the controller so the displays are
properly rooted below the hardware that drives them.
The frame buffer device also looks weird. Right now you only seem
to have a single frame buffer registered to a driver in the same
module. Is that frame buffer not dependent on a controller?
+#ifdef MODULE
+module_init(mcde_subsystem_init);
+#else
+fs_initcall(mcde_subsystem_init);
+#endif
This is not a file system ;-)
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
