On Tue, Jan 8, 2019 at 10:22 AM Andrzej Hajda <a.ha...@samsung.com> wrote: > > 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.
I'm pretty sure that most drivers do not hot-add/remove drm things after drm_dev_register (except drm_connector for dp mst support). It would be buggy. Do you have more examples? I haven't reviewed them all in detail, and things might have changed, so could very well be there's exceptions. > 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. There's no locking at all to handle bridges appearing/disappearing while the drm_device can be accessed. There's also no lifetime management/refcounting. So it "works" but it's racy like mad. > > 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 :) There's no drm_connector_get/put in exynos (except the one in hdmi_mode_fixup, which looks rather fishy tbh). And drm_connector are the only things in drm you can hotplug/unplug (except the overall drm_device ofc). So again it maybe "works", but you're guaranteed to have lots of races all over the place. -Daniel > > > 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 > >>> > >>> > > > -- 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
