Message-ID: <YFEnKgwEOWdeQBK6@pendragon.ideasonboard.com>
Date:   Tue, 16 Mar 2021 23:46:18 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Doug Anderson <dianders@...omium.org>
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@...r.kernel.org, robdclark@...omium.org,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but
 only if refclk

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.

> 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.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists