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
