Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Fri, Apr 25, 2014 at 04:36:01PM +0200, Andrzej Hajda wrote: On 04/23/2014 07:13 PM, Russell King - ARM Linux wrote: Let me be absolutely clear *why* I'm very interested in this - and that is because I'm presently converting TDA998x and Armada DRM to use the component helpers. If your solution is better, then I'd want to convert to that instead, and let's retire the component helpers. At the moment, my belief is that your solution is *very* substandard and suboptimal precisely for the reasons I've outlined, especially when it comes to sharing a component between several drivers. So, if you *really* care, you'll stop fobbing me off on this point and provide some real technical input how you'd see your solution being used in exactly the scenario that I've been outlining several times in this thread. For example, you could show what kind of modifications you expect would be required to the _existing_ TDA998x driver to allow it to participate as a device declared in DT as an entirely separate entity, probed via the standard I2C probing methods, and then hook itself into the appropriate DRM driver. Remembering, of course, that the TDA998x device is used by more than _just_ Armada DRM. I don't care if you show it via pseudo-code or by real patch. I just want to know _how_ your solution could be used. And I won't want some silly remark like trivially or I've already answered that. I want _you_ to _show_ _how_ it can be done. Thats good, constructive discussion is better. Well, the above is the _same_ fscking question I've been asking you all along and that you have failed to answer properly until now. Unfortunately, your reply still doesn't answer my concerns. I have no experience with tda998x, armada and drm_encoder_slave, so please be kind if I make some mistakes regarding them. I will try to show the complete solution starting from the current state of tda998x and armada in linux-next. I will present solution for DT as it is more standardized than board files. I hope it should not be a problem to port it back to board files if necessary. First I will try to show how to get rid of ifdefs in armada_drv.c, it is not a part of my proposition but you have emphasized it. 1. tda998x have already minimal DT support, so we can create proper i2c client device node with nxp,tda998x compatible. 2. There are tda998x_encoder_params defined in armada_drv.c they should be removed from armada and added to tda998x node, its parsing to tda driver should be added as well. 3. In armada_drv.c there is armada_drm_slave_config specific for tda. I am not sure of all its content but it seems it could be mapped to DT video interface. So if in armada_drm node there will be port node it means there is something connected to the output and it uses drm_encoder_slave interface, if there is no port node there is no encoder. Sample bindings: armada_drm { ... port { drm_ep: endpoint { remote-endpoint = tda_ep; crtcs = 1; poll_connect; poll_disconnect; interlace_allowed; }; }; }; ... i2c_x { tda@70 { reg = 0x70; compatible = nxp,tda998x; swap_a = ...; swap_b = ...; ... port { endpoint { remote-endpoint = drm_ep; }; }; }; }; 4. In armada_drm call of drm_i2c_encoder_init should be replaced by sth like: client = of_find_i2c_device_by_node(dev node containing drm_ep phandle); device_lock(client-dev); if (!client-dev.driver) { ret = -EPROBE_DEFER; goto unlock; } module_get(client-dev.driver-owner); encoder_drv = to_drm_i2c_encoder_driver(to_i2c_driver(client-dev.driver)); encoder_drv-encoder_init(client, dev, encoder); unlock: device_unlock(client-dev); Similar change should be done to destroy code. Of course please consider this code as a draft. This looks reasonable - and I'm sure we can easily work around the situation when there's no i2c encoder. There is one thing which concerns me about the above - that module_get() call. module_get() only saves you from the module being unloaded, which really isn't an issue if the problem with the device going away is solved. If you think about this for a moment, you'll understand why - in order for the module to be unloaded, the device driver must be first unbound from any devices. So: (a) you need to prevent the device driver being unbound while the DRM device is being brought up, and (b) you need to bring the DRM device down if the device driver is unbound. Those are the two difficult problems which need solving. The other thing which I
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On 04/23/2014 07:13 PM, Russell King - ARM Linux wrote: On Wed, Apr 23, 2014 at 05:43:28PM +0100, Russell King - ARM Linux wrote: So, maybe you would like to finally address *my* point about TDA998x and your solution in a way that provides a satisfactory answer. *Show* how it can be done, or *outline* how it can be done. Let me be absolutely clear *why* I'm very interested in this - and that is because I'm presently converting TDA998x and Armada DRM to use the component helpers. If your solution is better, then I'd want to convert to that instead, and let's retire the component helpers. At the moment, my belief is that your solution is *very* substandard and suboptimal precisely for the reasons I've outlined, especially when it comes to sharing a component between several drivers. So, if you *really* care, you'll stop fobbing me off on this point and provide some real technical input how you'd see your solution being used in exactly the scenario that I've been outlining several times in this thread. For example, you could show what kind of modifications you expect would be required to the _existing_ TDA998x driver to allow it to participate as a device declared in DT as an entirely separate entity, probed via the standard I2C probing methods, and then hook itself into the appropriate DRM driver. Remembering, of course, that the TDA998x device is used by more than _just_ Armada DRM. I don't care if you show it via pseudo-code or by real patch. I just want to know _how_ your solution could be used. And I won't want some silly remark like trivially or I've already answered that. I want _you_ to _show_ _how_ it can be done. Thats good, constructive discussion is better. I have no experience with tda998x, armada and drm_encoder_slave, so please be kind if I make some mistakes regarding them. I will try to show the complete solution starting from the current state of tda998x and armada in linux-next. I will present solution for DT as it is more standardized than board files. I hope it should not be a problem to port it back to board files if necessary. First I will try to show how to get rid of ifdefs in armada_drv.c, it is not a part of my proposition but you have emphasized it. 1. tda998x have already minimal DT support, so we can create proper i2c client device node with nxp,tda998x compatible. 2. There are tda998x_encoder_params defined in armada_drv.c they should be removed from armada and added to tda998x node, its parsing to tda driver should be added as well. 3. In armada_drv.c there is armada_drm_slave_config specific for tda. I am not sure of all its content but it seems it could be mapped to DT video interface. So if in armada_drm node there will be port node it means there is something connected to the output and it uses drm_encoder_slave interface, if there is no port node there is no encoder. Sample bindings: armada_drm { ... port { drm_ep: endpoint { remote-endpoint = tda_ep; crtcs = 1; poll_connect; poll_disconnect; interlace_allowed; }; }; }; ... i2c_x { tda@70 { reg = 0x70; compatible = nxp,tda998x; swap_a = ...; swap_b = ...; ... port { endpoint { remote-endpoint = drm_ep; }; }; }; }; 4. In armada_drm call of drm_i2c_encoder_init should be replaced by sth like: client = of_find_i2c_device_by_node(dev node containing drm_ep phandle); device_lock(client-dev); if (!client-dev.driver) { ret = -EPROBE_DEFER; goto unlock; } module_get(client-dev.driver-owner); encoder_drv = to_drm_i2c_encoder_driver(to_i2c_driver(client-dev.driver)); encoder_drv-encoder_init(client, dev, encoder); unlock: device_unlock(client-dev); Similar change should be done to destroy code. Of course please consider this code as a draft. All the above is just for removing ifdefs from armada_drv.c, it is not really connected with my proposition. It is not really safe, and I am not sure where exactly the locking should be performed. For sure it can crash when unbind will be called via sysfs property, but it seems at least as safe as drm_i2c_encoder_init or it should be possible to make it such. And it allows to attach to armada theoretically any hardware compatible encoder having drm_i2c_encoder interface. What do you think about above steps? Is it OK for you? And now about my proposition for device probe order issue. My first answer to your objections about 'glue problem' was to make global list of 'ready' devices instead of the one glued to the main drm driver. Over time my idea evolved. My
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On 04/22/2014 01:51 PM, Russell King - ARM Linux wrote: On Tue, Apr 22, 2014 at 01:29:54PM +0200, Andrzej Hajda wrote: On 04/18/2014 02:46 PM, Russell King - ARM Linux wrote: On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote: Separation of the interfaces exposed by the device from the device itself seems to me a good thing. I would even consider it as a biggest advantage of this solution :) The problem of re-initialization does not seems to be relevant here, it is classic problem of coding correctness nothing more, it can appear here and in many different places. It may be a problem of coding correctless, but it's also a maintainability problem too - it makes it _much_ more difficult to ensure that everything is correct. But forcibly re-initializing all component devices instead of fixing bugs in specific drivers seems to be 'absolutely absurd' as classic says. They're *unnecessary* bugs that wouldn't even exist if it weren't for the forced-splitup of the initialisation into two separate parts that your approach mandates. Yes, I know that you're desperate to play that down, but you can't get Not true. I only try to find the best solution and the approach with multiple re-probing just to avoid potential bugs in drivers does not look to me correct. away from this fact: your approach _forces_ a split up of the initialisation into dependent two stages and that fact _alone_ adds additional complexity, and along with that additional complexity comes more opportunity for bugs. This sound so funny, just look at componentize patches - every patch adds two stage initialization for every component and the master, with forced unwinding and two levels of devres management. 'My approach' adds only one call to probe and one call to remove of components, and very simple and straightforward interface to the master. 'My approach' is very standard - during probe driver probes hardware, and registers interfaces which can be used by other drivers, for example by drm master. The only addition is reporting its readiness. Comparing to 'your approach' it is bloody simple. Also with that additional complexity comes the need to perform more tests to find those bugs, and given that most people just say okay, it boots and seems to work, that's good enough for me there is a high possibility that these kinds of bugs will take a long time to find. Volume of changes for each component and drm device management dispersed on all components makes your argument very valid for component subsystem. Btw have you observed component framework when one of the components during bind returns -EPROBE_DEFER ? In my tests it resulted in deferred probing of master and unbind/bind of other components. So lets say you have N components and the last component will be deferred K times, it results in: - K times deferring of the last component and the master, - (N - 1) * K - unbinds and binds of other components. As I wrote already, this framework was proposed for drivers which are tied together anyway, and this is case of many drivers, not only exynos. Please name them. Standalone drivers were not at my sight but I have also described in other mail how the framework can be 'improved' to support standalone drivers also. At the moment, I don't see a justification for your simplified and restrictive solution, which if used will lock drivers into that simplisitic method, and which can't ever be extended without lots of ifdeffery to having other components (such as TDA998x) attached. My objections are entirely based on where imx-drm and armada DRM are going, neither of which could ever use your implementation. Before you say that it isn't meant to deal with stuff like the TDA998x, take a moment to think about this - the Dove video subsystem was designed to support OLPC. It was primerly designed to drive a LCD screen plus an on-board VGA DAC. Everything for that is on-SoC. With that, the hardware is well known, and your solution could be used. However, then SolidRun came along and dropped a TDA998x on the LCD output pins. Suddenly, things aren't that simple, and your solution falls apart, because it can't cope with a generic component that has no knowledge of the rest of its master. This kind of scenario can happen /any/ time, and any time it does happen, your simple solution falls apart. I think I have answered you two or three times that it is not a problem to remove 'glued drivers' restriction. I desperately try to avoid accusing you for 'desperately playing down' on this subject, so I will not comment this anymore. On the other hand you have not answered quite important question - how do you plan to componentize drivers shared by different drms when one of drms is not componentized??? Regards Andrzej -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Wed, Apr 23, 2014 at 05:04:46PM +0200, Andrzej Hajda wrote: On 04/22/2014 01:51 PM, Russell King - ARM Linux wrote: Yes, I know that you're desperate to play that down, but you can't get Not true. I only try to find the best solution and the approach with multiple re-probing just to avoid potential bugs in drivers does not look to me correct. away from this fact: your approach _forces_ a split up of the initialisation into dependent two stages and that fact _alone_ adds additional complexity, and along with that additional complexity comes more opportunity for bugs. This sound so funny, just look at componentize patches - every patch adds two stage initialization for every component and the master, with forced unwinding and two levels of devres management. *Sigh*. Why am I bothering discussing this with you. *NO* it does not, for the VERY SIMPLE reason that NOTHING is done before the BIND. NO structures are allocated. NOTHING is setup. The *only* thing that is done is the driver registers with the component helper. That's not two stage initialisation. That's *single* stage. 'My approach' adds only one call to probe and one call to remove of components, and very simple and straightforward interface to the master. You're talking utter garbage there. 'My approach' is very standard - during probe driver probes hardware, and registers interfaces which can be used by other drivers, for example by drm master. The only addition is reporting its readiness. Comparing to 'your approach' it is bloody simple. More unbelievable crap. Also with that additional complexity comes the need to perform more tests to find those bugs, and given that most people just say okay, it boots and seems to work, that's good enough for me there is a high possibility that these kinds of bugs will take a long time to find. Volume of changes for each component and drm device management dispersed on all components makes your argument very valid for component subsystem. Btw have you observed component framework when one of the components during bind returns -EPROBE_DEFER ? In my tests it resulted in deferred probing of master and unbind/bind of other components. So lets say you have N components and the last component will be deferred K times, it results in: - K times deferring of the last component and the master, - (N - 1) * K - unbinds and binds of other components. True, and you can't get away from that with proper behaviour. As I wrote already, this framework was proposed for drivers which are tied together anyway, and this is case of many drivers, not only exynos. Please name them. You ignored this. Therefore, I assume that you *can't* name them because there *aren't* any. I called your bluff, I win. At the moment, I don't see a justification for your simplified and restrictive solution, which if used will lock drivers into that simplisitic method, and which can't ever be extended without lots of ifdeffery to having other components (such as TDA998x) attached. My objections are entirely based on where imx-drm and armada DRM are going, neither of which could ever use your implementation. Before you say that it isn't meant to deal with stuff like the TDA998x, take a moment to think about this - the Dove video subsystem was designed to support OLPC. It was primerly designed to drive a LCD screen plus an on-board VGA DAC. Everything for that is on-SoC. With that, the hardware is well known, and your solution could be used. However, then SolidRun came along and dropped a TDA998x on the LCD output pins. Suddenly, things aren't that simple, and your solution falls apart, because it can't cope with a generic component that has no knowledge of the rest of its master. This kind of scenario can happen /any/ time, and any time it does happen, your simple solution falls apart. I think I have answered you two or three times that it is not a problem to remove 'glued drivers' restriction. I desperately try to avoid accusing you for 'desperately playing down' on this subject, so I will not comment this anymore. Right, so what I draw from this is that *you* again refuse to answer this point because despite your assertions that your solution can do it, you have no clue as to *how* it can be done. I've looked at your solution with respect to this, and I *can't* see how it can be done either. That's why I've been asking *you* the question, so that *you* can provide some technical input to it. On the other hand you have not answered quite important question - how do you plan to componentize drivers shared by different drms when one of drms is not componentized??? Read this, from a message I sent at the beginning of February: | Here's my changes to the TDA998x driver to add support for the component | helper. The TDA998x driver retains support for the old way so that | drivers can be transitioned. For any one DRM
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Wed, Apr 23, 2014 at 05:43:28PM +0100, Russell King - ARM Linux wrote: So, maybe you would like to finally address *my* point about TDA998x and your solution in a way that provides a satisfactory answer. *Show* how it can be done, or *outline* how it can be done. Let me be absolutely clear *why* I'm very interested in this - and that is because I'm presently converting TDA998x and Armada DRM to use the component helpers. If your solution is better, then I'd want to convert to that instead, and let's retire the component helpers. At the moment, my belief is that your solution is *very* substandard and suboptimal precisely for the reasons I've outlined, especially when it comes to sharing a component between several drivers. So, if you *really* care, you'll stop fobbing me off on this point and provide some real technical input how you'd see your solution being used in exactly the scenario that I've been outlining several times in this thread. For example, you could show what kind of modifications you expect would be required to the _existing_ TDA998x driver to allow it to participate as a device declared in DT as an entirely separate entity, probed via the standard I2C probing methods, and then hook itself into the appropriate DRM driver. Remembering, of course, that the TDA998x device is used by more than _just_ Armada DRM. I don't care if you show it via pseudo-code or by real patch. I just want to know _how_ your solution could be used. And I won't want some silly remark like trivially or I've already answered that. I want _you_ to _show_ _how_ it can be done. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
Hi Russel, My answer little bit later due to Easter. On 04/18/2014 02:42 PM, Russell King - ARM Linux wrote: On Fri, Apr 18, 2014 at 01:27:53PM +0200, Andrzej Hajda wrote: Hi Russel, Thanks for comments. On 04/17/2014 11:47 PM, Russell King - ARM Linux wrote: On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote: +out: + if (ret != -EPROBE_DEFER) + exynos_drm_dev_ready(pdev-dev); So we end up with everyone needing a ready call in each sub-driver back into the main driver... this makes it impossible to write a generic subcomponent driver which is not tied in any way to the main driver. That is quite some restriction, and would prevent, for example, the TDA998x driver being used both with Armada DRM, tilcdc and any other driver. As I see in armada driver drm is deferred in case tda998x is not yet available. The same solution can be still used with pending_devices approach - the main driver will not report its readiness until tda998x is present. Anyway it still seems to be better than componentize every driver which can probably become a part of some superdevice. If you want to get rid of deferred probe one can make global list of 'ready' devices with notifications systems for master devices. Maybe it would be good to consider notification system for devices probe result, it will require that driver register all its interfaces in probe, ie its readiness cannot be reported later but will not require to add new framework. I hope just extending current notification system should be enough. You aren't addressing my point. If I were to convert tda998x to use your infrastructure, then I would have to add in ifdefs to tie it into armada DRM _and_ a different set of ifdefs to tie it into tilcdc. Then when someone else wanted to use it in their driver, they'd have to add yet more ifdefs into it to tie it into their driver. This does not scale. As I already answered, you should not use 'my' framework for tda998x driver, you can still use current approach with deferred probe. I am not sure why have you used ifdefs in armada, tilcdc also uses tda998x and without ifdefs. 'My' framework (I think helper library is a better name) was created to use with devices which are closely tied together by another framework - case of some SoC devices. So, please address my point: in your system, how can a single component be shared between multiple different master drivers? I have answered this question above, again. But your question suggests you want to componentize also drivers which are shared by different DRMs. How do you want to do it? - componentize all DRM drivers sharing given driver? - componentize shared device in a way that it can used by non-componentized devices? how? I guess it will be possible but will have some price. Regards Andrzej -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On 04/18/2014 02:46 PM, Russell King - ARM Linux wrote: On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote: Separation of the interfaces exposed by the device from the device itself seems to me a good thing. I would even consider it as a biggest advantage of this solution :) The problem of re-initialization does not seems to be relevant here, it is classic problem of coding correctness nothing more, it can appear here and in many different places. It may be a problem of coding correctless, but it's also a maintainability problem too - it makes it _much_ more difficult to ensure that everything is correct. But forcibly re-initializing all component devices instead of fixing bugs in specific drivers seems to be 'absolutely absurd' as classic says. Anyway it seems we have different point of view on the problem, your say about devices with two stage initialization. I see it more as devices registering interfaces and superdevice using it. Right, so please make this exynos-specific, because from what I can see it has no reason to pretend to be generic. As I've already pointed out, it can't be used in the general case because it ties sub-components directly to their main driver, which is absolutely absurd. Please keep this absurdness in exynos and don't spread it around. Thanks. As I wrote already, this framework was proposed for drivers which are tied together anyway, and this is case of many drivers, not only exynos. Standalone drivers were not at my sight but I have also described in other mail how the framework can be 'improved' to support standalone drivers also. Regards Andrzej -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Tue, Apr 22, 2014 at 01:29:54PM +0200, Andrzej Hajda wrote: On 04/18/2014 02:46 PM, Russell King - ARM Linux wrote: On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote: Separation of the interfaces exposed by the device from the device itself seems to me a good thing. I would even consider it as a biggest advantage of this solution :) The problem of re-initialization does not seems to be relevant here, it is classic problem of coding correctness nothing more, it can appear here and in many different places. It may be a problem of coding correctless, but it's also a maintainability problem too - it makes it _much_ more difficult to ensure that everything is correct. But forcibly re-initializing all component devices instead of fixing bugs in specific drivers seems to be 'absolutely absurd' as classic says. They're *unnecessary* bugs that wouldn't even exist if it weren't for the forced-splitup of the initialisation into two separate parts that your approach mandates. Yes, I know that you're desperate to play that down, but you can't get away from this fact: your approach _forces_ a split up of the initialisation into dependent two stages and that fact _alone_ adds additional complexity, and along with that additional complexity comes more opportunity for bugs. Also with that additional complexity comes the need to perform more tests to find those bugs, and given that most people just say okay, it boots and seems to work, that's good enough for me there is a high possibility that these kinds of bugs will take a long time to find. As I wrote already, this framework was proposed for drivers which are tied together anyway, and this is case of many drivers, not only exynos. Please name them. Standalone drivers were not at my sight but I have also described in other mail how the framework can be 'improved' to support standalone drivers also. At the moment, I don't see a justification for your simplified and restrictive solution, which if used will lock drivers into that simplisitic method, and which can't ever be extended without lots of ifdeffery to having other components (such as TDA998x) attached. My objections are entirely based on where imx-drm and armada DRM are going, neither of which could ever use your implementation. Before you say that it isn't meant to deal with stuff like the TDA998x, take a moment to think about this - the Dove video subsystem was designed to support OLPC. It was primerly designed to drive a LCD screen plus an on-board VGA DAC. Everything for that is on-SoC. With that, the hardware is well known, and your solution could be used. However, then SolidRun came along and dropped a TDA998x on the LCD output pins. Suddenly, things aren't that simple, and your solution falls apart, because it can't cope with a generic component that has no knowledge of the rest of its master. This kind of scenario can happen /any/ time, and any time it does happen, your simple solution falls apart. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On 04/18/2014 12:04 AM, Russell King - ARM Linux wrote: On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote: +static int exynos_drm_add_blocker(struct device *dev, void *data) +{ +struct device_driver *drv = data; + +if (!platform_bus_type.match(dev, drv)) +return 0; + +device_lock(dev); +if (!dev-driver) +exynos_drm_dev_busy(dev); +device_unlock(dev); + +return 0; +} + +static void exynos_drm_init_blockers(struct device_driver *drv) +{ +bus_for_each_dev(platform_bus_type, NULL, drv, exynos_drm_add_blocker); +} This feels very wrong to be dumping the above code into every driver which wants to make use of some kind of componentised support. You also appear to need to know the struct device_driver's for every component. While that may work for exynos, it doesn't work elsewhere where the various components of the system are very real separate kernel modules - for example, a separate I2C driver such as the TDA998x case I mentioned in my first reply. I can't see how your solution would be usable in that circumstance. It is up to the master driver how it want to create list of required devices, this is why I put it into exynos_drm and not into the framework. You can use superdevice DT node for it, fixed list whatever you want. It is not a part of the framework, it is just part of exynos_drm specific implementation. Component framework also does not provide mechanism for it. Regarding TDA998x I have replied in the previous e-mail. The third issue I have is that you're still needing to have internal exynos sub-device management - you're having to add the individual devices to some kind of list, array or static data, and during DRM probe you're having to then walk these lists/arrays/static data to locate these sub-devices and finish each of their individual initialisations. So you're ending up with a two-tier initialisation. That's not particularly good because it means you're exposed to problems where the state is different between two initialisations - when the device is recreated, your component attempts to re-finalise the initialisation a second time. It wouldn't take much for a field to be assumed to be zero at init time somewhere for a bug to creep in. Separation of the interfaces exposed by the device from the device itself seems to me a good thing. I would even consider it as a biggest advantage of this solution :) The problem of re-initialization does not seems to be relevant here, it is classic problem of coding correctness nothing more, it can appear here and in many different places. Anyway it seems we have different point of view on the problem, your say about devices with two stage initialization. I see it more as devices registering interfaces and superdevice using it. Regards Andrzej -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Fri, Apr 18, 2014 at 01:27:53PM +0200, Andrzej Hajda wrote: Hi Russel, Thanks for comments. On 04/17/2014 11:47 PM, Russell King - ARM Linux wrote: On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote: +out: + if (ret != -EPROBE_DEFER) + exynos_drm_dev_ready(pdev-dev); So we end up with everyone needing a ready call in each sub-driver back into the main driver... this makes it impossible to write a generic subcomponent driver which is not tied in any way to the main driver. That is quite some restriction, and would prevent, for example, the TDA998x driver being used both with Armada DRM, tilcdc and any other driver. As I see in armada driver drm is deferred in case tda998x is not yet available. The same solution can be still used with pending_devices approach - the main driver will not report its readiness until tda998x is present. Anyway it still seems to be better than componentize every driver which can probably become a part of some superdevice. If you want to get rid of deferred probe one can make global list of 'ready' devices with notifications systems for master devices. Maybe it would be good to consider notification system for devices probe result, it will require that driver register all its interfaces in probe, ie its readiness cannot be reported later but will not require to add new framework. I hope just extending current notification system should be enough. You aren't addressing my point. If I were to convert tda998x to use your infrastructure, then I would have to add in ifdefs to tie it into armada DRM _and_ a different set of ifdefs to tie it into tilcdc. Then when someone else wanted to use it in their driver, they'd have to add yet more ifdefs into it to tie it into their driver. This does not scale. So, please address my point: in your system, how can a single component be shared between multiple different master drivers? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Fri, Apr 18, 2014 at 02:02:37PM +0200, Andrzej Hajda wrote: Separation of the interfaces exposed by the device from the device itself seems to me a good thing. I would even consider it as a biggest advantage of this solution :) The problem of re-initialization does not seems to be relevant here, it is classic problem of coding correctness nothing more, it can appear here and in many different places. It may be a problem of coding correctless, but it's also a maintainability problem too - it makes it _much_ more difficult to ensure that everything is correct. Anyway it seems we have different point of view on the problem, your say about devices with two stage initialization. I see it more as devices registering interfaces and superdevice using it. Right, so please make this exynos-specific, because from what I can see it has no reason to pretend to be generic. As I've already pointed out, it can't be used in the general case because it ties sub-components directly to their main driver, which is absolutely absurd. Please keep this absurdness in exynos and don't spread it around. Thanks. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote: +out: + if (ret != -EPROBE_DEFER) + exynos_drm_dev_ready(pdev-dev); So we end up with everyone needing a ready call in each sub-driver back into the main driver... this makes it impossible to write a generic subcomponent driver which is not tied in any way to the main driver. That is quite some restriction, and would prevent, for example, the TDA998x driver being used both with Armada DRM, tilcdc and any other driver. So, while your solution may work for exynos, it's not suitable for general use. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/3] drm/exynos: use pending_components for components tracking
On Thu, Apr 17, 2014 at 01:28:50PM +0200, Andrzej Hajda wrote: +static int exynos_drm_add_blocker(struct device *dev, void *data) +{ + struct device_driver *drv = data; + + if (!platform_bus_type.match(dev, drv)) + return 0; + + device_lock(dev); + if (!dev-driver) + exynos_drm_dev_busy(dev); + device_unlock(dev); + + return 0; +} + +static void exynos_drm_init_blockers(struct device_driver *drv) +{ + bus_for_each_dev(platform_bus_type, NULL, drv, exynos_drm_add_blocker); +} This feels very wrong to be dumping the above code into every driver which wants to make use of some kind of componentised support. You also appear to need to know the struct device_driver's for every component. While that may work for exynos, it doesn't work elsewhere where the various components of the system are very real separate kernel modules - for example, a separate I2C driver such as the TDA998x case I mentioned in my first reply. I can't see how your solution would be usable in that circumstance. The third issue I have is that you're still needing to have internal exynos sub-device management - you're having to add the individual devices to some kind of list, array or static data, and during DRM probe you're having to then walk these lists/arrays/static data to locate these sub-devices and finish each of their individual initialisations. So you're ending up with a two-tier initialisation. That's not particularly good because it means you're exposed to problems where the state is different between two initialisations - when the device is recreated, your component attempts to re-finalise the initialisation a second time. It wouldn't take much for a field to be assumed to be zero at init time somewhere for a bug to creep in. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html