On 08.01.2019 09:47, Daniel Vetter wrote: > On Tue, Jan 8, 2019 at 9:35 AM Andrzej Hajda <a.ha...@samsung.com> wrote: >> On 07.01.2019 22:56, Daniel Vetter wrote: >>> On Mon, Jan 7, 2019 at 5:27 PM Andrzej Hajda <a.ha...@samsung.com> wrote: >>>> On 07.01.2019 17:08, Daniel Vetter wrote: >>>>> On Mon, Jan 07, 2019 at 12:26:58PM +0100, Andrzej Hajda wrote: >>>>>> On 07.01.2019 11:45, Daniel Vetter wrote: >>>>>>> On Thu, Jan 03, 2019 at 01:11:47PM +0000, Russell King - ARM Linux >>>>>>> wrote: >>>>>>>> On Thu, Jan 03, 2019 at 10:47:27AM +0100, Lubomir Rintel wrote: >>>>>>>>> Hello, >>>>>>>>> >>>>>>>>> lately I've been trying to make the Himax HX8837 chip that drives the >>>>>>>>> OLPC >>>>>>>>> LCD display work with Armada DRM driver. I've been advised to create a >>>>>>>>> bridge driver and not an encoder driver since the silicon is separate >>>>>>>>> from >>>>>>>>> the LCDC. >>>>>>>>> >>>>>>>>> The Armada DRM driver (and, I think, the i.MX one) creates the >>>>>>>>> drm_device >>>>>>>>> once the component infrastructure sees the necessary sub-devices >>>>>>>>> appear. >>>>>>>>> The sub-devices being the LCDCs and the encoders (not bridges) that it >>>>>>>>> expects to be created externally. >>>>>>>>> >>>>>>>>> Currently, it seems, the only driver that can actually work with this >>>>>>>>> (that >>>>>>>>> is -- creates a drm_encoder for a drm_device when the component is >>>>>>>>> bound) >>>>>>>>> is the tda998x. All other similar drivers create a drm_bridge instead >>>>>>>>> and >>>>>>>>> not use the component infrastructure at all. (In fact, tilcdc driver >>>>>>>>> contains a hack to handle tda998x specially.) >>>>>>>>> >>>>>>>>> I'm wondering how to reconcile the two? >>>>>>>>> >>>>>>>>> * The tda998x driver has recently been modified to create a bridge on >>>>>>>>> probe >>>>>>>>> and eventually encoder on component bind. Is this an okay thing to >>>>>>>>> do in >>>>>>>>> a new driver? (this probably means the tilcdc hack can be >>>>>>>>> removed...) >>>>>>>>> >>>>>>>>> * If a non-componentized bridge were to be used (along with a dummy >>>>>>>>> encoder), at what point would it make sense to look for the bridge? >>>>>>>>> Would it be a good idea to defer the probe of crtc until a bridge >>>>>>>>> can be >>>>>>>>> looked up and the attach it on component bind? What if the bridge >>>>>>>>> goes >>>>>>>>> away (a module is unloaded, etc.) in between? >>>>>>>>> >>>>>>>>> I'd be thankful for opintions and advice before I move ahead with >>>>>>>>> this. >>>>>>>> This is the long-standing problem with the conflict between bridge >>>>>>>> support and component support, and I'm not sure that there is really >>>>>>>> any answer to it. >>>>>>>> >>>>>>>> I've gone into the details of the two several times on the list, >>>>>>>> particularly about the short-comings of the bridge approach, but it >>>>>>>> seems no one cares to fix those short-comings. >>>>>>>> >>>>>>>> You are re-identifying some of the issues that I've already pointed >>>>>>>> out - such as what happens to DRM drives when the bridge driver is >>>>>>>> unbound (it's really not about modules being unloaded, and the problem >>>>>>>> can't be solved by taking a module reference count - all that the >>>>>>>> module reference count does is ensure that the module doesn't go >>>>>>>> away unexpected, there is no way to ensure that the device isn't >>>>>>>> unbound.) >>>>>>>> >>>>>>>> The issue of unbinding is precisely the issue which the component >>>>>>>> support was created to solve - but everyone seems to prefer the buggy >>>>>>>> bridge approach, and no one seems willing to do anything about the >>>>>>>> bugs or even acknowledge that it's a problem. It's strange - if one >>>>>>>> identifies bugs that result in kernel oops in other kernel subsystems, >>>>>>>> one is generally taken seriously and the problem is solved. >>>>>>> Unbinding is really not the most important feature, especially for SoC. >>>>>>> If >>>>>>> you feel different, working together with others, getting some >>>>>>> agreement, >>>>>>> getting the patches reviewed and finding someone to get them merged is >>>>>>> very much appreciated. But just complaining won't move this forward. >>>>>>> >>>>>>>> The issue about the encoders is something that I've tried to discuss, >>>>>>>> and I've pointed out that moving it into the DRM driver adds additional >>>>>>>> complexity there, and I'd hoped that my patch set I posted would've >>>>>>>> generated discussion, but alas not. >>>>>>>> >>>>>>>> What I'm not prepared to do is to introduce _known_ bugs into any >>>>>>>> driver that I maintain. >>>>>>> I thought last time around the idea was to use device links (and teach >>>>>>> drm_bridge about them), so that the unloading works correctly. >>>>>> With current device_links implementation registering links in probe of >>>>>> the consumer is just incorrect - it can happen that neither consumer >>>>>> neither provider is fully probed and creating device links in such state >>>>>> is wrong according to docs, and my experiments. See [1] for discussion >>>>>> and [2] for docs. >>>>> We could set up the device link only at drm_dev_register time. At that >>>>> point >>>>> we know that driver loading has fully succeeded. We do have a list of >>>>> bridges at hand, so that's not going to be an issue. >>>>> >>>>> The only case where this breaks is if a driver is still using the >>>>> deprecated ->load callback. But that ->load callback doesn't mix well with >>>>> EDEFER_PROBE/component framework anyway, so I think not going to be a >>>>> problem hopefully? >>>> drm_dev_register usually is called from bind callback, which is called >>>> from probe callback of one of the components or master (depending on >>>> particular probe order). If you want to register device link in this >>>> function it is possible that the bad scenario will happen - there will be >>>> attempt to create device link between not-yet-probed consumer and >>>> not-yet-probed provider. >>> If you call drm_dev_register before you have all the components >>> assembled (i.e. anywhere else than in master bind) that sounds like a >>> driver bug. >> >> This is how components work, every component calls component_add usually >> in probe, and this function checks if all components are present, if yes >> (ie. during probe of the last component) master bind is called and it >> creates drm device and then registers it. If you add device link at this >> moment, it is still before end of probe of the last component. >> >> On the other side provider is registered (drm_bridge_add in case of >> bridges) during its probe so it becomes available to the consumers >> BEFORE end of its probe and again it can happen that device link will be >> added to early. > Hm I read that thread again. Is the reason why we can't add the device > link only the exynos behaviour to allow drm_panel to be > hot-added/removed?
Not only. What I have described above is common to all drivers it has nothing specific to Exynos. Of course it was tested on Exynos as this is HW I work on. Linus Walleij has also reported in this thread that device links have different issue - circular dependencies (last few emails in this lengthy thread). My response explains it in detail. > > Note that for bridge allowing this is 100% buggy: drm core does not > allow you to hotplug/hotunplug bridges. What part of drm core disallows it? As I remember discussions about drm_bridge design there were voices that they should be hot(un)pluggable, and they are IMO, of course if they are not active. > > In theory drm_panel can be hotplugged (because we can hotplug > drm_connector), but I'm pretty sure exynos gets it all wrong since > hotplugging panels is no easy thing to do. I don't think this is > something we want to support, forcing the entire driver to be unload > sounds like what we want to do here. I do not know if it is 'all wrong', but tests shows it is hot(un)pluggable. In both cases of panel and bridge :) Regards Andrzej > -Daniel > >> >> Regards >> >> Andrzej >> >> >>> drm_dev_register publishes the drm device instance to the >>> world, if you're not ready to handle userspace requests at that point >>> (because not everything is loaded yet) then things will go boom in >>> very colorful ways. And from my (admittedly very rough) understanding >>> we should be able to register the the device links as the very last >>> step in the master bind function (and drm_dev_register should be about >>> the last thing you do in the master bind). >>> >>> So not clear on why this won't work? >> >> >> >>> -Daniel >>> -- >>> Daniel Vetter >>> Software Engineer, Intel Corporation >>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch >>> >>> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
