Message-ID: <CAD=FV=W5fpyEf4AqJ+dZ7i_rD_PE40MyNsYNydhPi4BHkEfQcQ@mail.gmail.com>
Date:   Tue, 16 Mar 2021 17:44:59 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Sam Ravnborg <sam@...nborg.org>,
        Stephen Boyd <swboyd@...omium.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Rob Clark <robdclark@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but
 only if refclk

Hi,

On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Doug,
>
> On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> > On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > > EDID from the panel. That commit kinda worked but it had some serious
> > > > problems.
> > > >
> > > > The problems all stem from the fact that userspace wants to be able to
> > > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > > though, we don't actually power the panel up until the pre-enable
> > > > stage and the pre-enable call happens right before the enable call
> > > > with no way to interject in-between. For eDP panels, you can't read
> > > > the EDID until you power the panel. The result was that
> > > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > > the EDID was needed.
> > > >
> > > > To make it concrete, on my system I saw this happen:
> > > > 1. We'd attach the bridge.
> > > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > > >    to read the EDID over and over again and fall back to the hardcoded
> > > >    modes.
> > > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > > 4. Userspace would ask to turn the panel on.
> > > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > >    this happens on the handoff from the boot splash screen to the
> > > >    browser). Now we'd read them properly and, if they were different,
> > > >    userspace would request to change the mode.
> > > >
> > > > The fact that userspace would always end up using the hardcoded modes
> > > > at first significantly decreases the benefit of the EDID
> > > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > > doing a wasteful modeset and at boot.
> > >
> > > s/and at/at/ ?
> >
> > Sure, I can correct if/when I respin or it can be corrected when landed.
> >
> > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > index 491c9c4f32d1..af3fb4657af6 100644
> > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > @@ -16,6 +16,7 @@
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <linux/regmap.h>
> > > >  #include <linux/regulator/consumer.h>
> > > > +#include <linux/workqueue.h>
> > > >
> > > >  #include <asm/unaligned.h>
> > > >
> > > > @@ -130,6 +131,12 @@
> > > >   * @ln_assign:    Value to program to the LN_ASSIGN register.
> > > >   * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
> > > >   *
> > > > + * @pre_enabled_early: If true we did an early pre_enable at attach.
> > > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from attach
> > > > + *                           if a normal pre_enable never came.
> > >
> > > Could we simplify this by using the runtime PM autosuspend feature ? The
> > > configuration of the bridge would be moved from pre_enable to the PM
> > > runtime resume handler, the clk_disable_unprepare() call moved from
> > > post_disable to the runtime suspend handler, and the work queue replaced
> > > by usage of pm_runtime_put_autosuspend().
> >
> > It's an interesting idea but I don't think I can make it work, at
> > least not in a generic enough way. Specifically we can also use this
> > bridge chip as a generic GPIO provider in Linux. When someone asks us
> > to read a GPIO then we have to power the bridge on
> > (pm_runtime_get_sync()) and when someone asks us to configure a GPIO
> > as an output then we actually leave the bridge powered until they stop
> > requesting it as an output. At the moment the only user of this
> > functionality (that I know of) is for the HPD pin on trogdor boards
> > (long story about why we don't use the dedicated HPD) but the API
> > supports using these GPIOs for anything and I've tested that it works.
> > It wouldn't be great to have to keep the panel on in order to access
> > the GPIOs.
>
> The issue you're trying to fix doesn't seem specific to this bridge, so
> handling it in the bridge driver bothers me :-S Is there any way we
> could handle this in the DRM core ? I don't want to see similar
> implementations duplicated in all HDMI/DP bridges.

Yes, it is true that this problem could affect other drivers.  ...and
in full disclosure I think there are other similar workarounds already
present. I haven't personally worked on those chips, but in
ps8640_bridge_get_edid() there is a somewhat similar workaround to
chain a pre-enable (though maybe it's not quite as optimized?). I'm
told that maybe something had to be handled for anx7625 (in
anx7625_get_edid()?) but that definitely doesn't look at all like it's
doing a pre-enable, so maybe things for that bridge just work
differently.

One thing that makes me hesitant about trying to moving this to the
core is that even in sn65dsi86 there is a case where it won't work. As
I mentioned in the patch I'm not aware of anyone using it in
production, but if someone was using the MIPI clock as input to the
bridge chip instead of a fixed "refclk" then trying to get the EDID
after just "pre-enable" falls over.  Said another way: I can say that
with this particular bridge chip, if you're using a fixed refclk, you
can read the EDID after the pre-enable. I don't know if that's always
true with all other bridge chips.

So I guess in summary: I think I could put my code in the core, but I
don't _think_ I can just make it automatically enabled.

* In sn65dsi I'd have to only enable it if we have a fixed refclk.

* Maybe in ps8640 I could just always enable it and replace the
existing code? I'd have to find someone to test.

* In anx7625 things look totally different.

Can you give me any advice on how you'd like me to proceed?

-Doug

Powered by blists - more mailing lists