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? Note that for bridge allowing this is 100% buggy: drm core does not allow you to hotplug/hotunplug bridges. 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. -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 > > > > > -- 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
