Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk

[Doug Anderson <dianders@chromium.org>]

Hi,

On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> 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 other problem is that I think the time scales are different. At
boot time I think we'd want to leave the panel on for tens of seconds
to give userspace a chance to start up and configure the panel. After
userspace starts up I think we'd want autosuspend to be much faster.
This could probably be solved by tweaking the runtime delay in code
but I didn't fully dig because of the above problem.


-Doug