Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Rob Clark]

On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda <a.hajda@xxxxxxxxxxx> wrote:
>
> On 29.08.2019 19:39, Rob Clark wrote:
> > On Wed, Aug 28, 2019 at 11:06 PM John Stultz <john.stultz@xxxxxxxxxx> wrote:
> >> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
> >> host at probe time") landed in -next the HiKey board would fail
> >> to boot, looping:
> > No, please revert 83f35bc3a852.. that is going in the *complete* wrong
> > direction.  We actually should be moving panels to not require dsi
> > host until attach time, similar to how bridges work, not the other way
> > around.
>
>
> Devices of panels and bridges controlled via DSI will not appear at all
> if DSI host is not created.
>
> So this is the only direction!!!
>

I disagree, there is really no harm in the bridge probing if there is no dsi.

Furthermore, it seems that this change broke a few other drivers.

> >
> > The problem is that, when dealing with bootloader enabled display, we
> > need to be really careful not to touch the hardware until the display
> > driver knows the bridge/panel is present.  If the bridge/panel probes
> > after the display driver, we could end up killing scanout
> > (efifb/simplefb).. if the bridge/panel is missing some dependency and
> > never probes, it is rather unpleasant to be stuck trying to debug what
> > went wrong with no display.
>
>
> It has nothing to do with touching hardware, you can always (I hope)
> postpone it till all components are present.

Unfortunately this is harder than it sounds, since we need to read hw
revision registers for display and dsi blocks to know which hw
revision we are dealing with.

(Also, we need to avoid
drm_fb_helper_remove_conflicting_framebuffers() until we know we are
ready to go.)

We could possibly put more information in dt.  But the less we depend
on dt, the easier time we'll have adding support for ACPI boot on the
windows arm laptops.

> But it is just requirement of device/driver model in Linux Kernel.

yes and no.. the way the existing bridges work with a bridge->attach()
step seems fairly pragmatic to me.

>
> >
> > Sorry I didn't notice that adv7511 patch before it landed, but the
> > right thing to do now is to revert it.
>
>
> The 1st version of the patch was posted at the end of April and final
> version was queued 1st July, so it was quite long time for discussions
> and tests.

sorry I didn't notice the patch sooner, most of my bandwidth goes to mesa.

> Reverting it now seems quite late, especially if the patch does right
> thing and there is already proper fix for one encoder (kirin), moreover
> revert will break another platforms.

kirin isn't the only other user of adv75xx.. at least it is my
understanding that this broke db410c as well.

> Of course it seems you have different opinion what is the right thing in
> this case, so if you convince us that your approach is better one can
> revert the patch.

I guess my strongest / most immediate opinion is to not break other
existing adv75xx bridge users.

Beyond that, I found doing mipi_dsi_attach() in bridge->attach() was
quite convenient to get display handover from efifb work.  And that
was (previously) the way most of the bridges worked.

But maybe there is another way.. perhaps somehow the bridges/panels
could be added as sub-components using the component framework, to
prevent the toplevel drm device from probing until they are ready?
We'd still have the problem that the dsi component wants to be able to
read hw revision before registering dsi host.. but I suppose if CCF
exported a way that we could query whether the interface clk was
already enabled, we could have the clk enable/disable cycle that would
break efifb.

BR,
-R