All of lore.kernel.org
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Doug Anderson <dianders@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Sam Ravnborg <sam@ravnborg.org>,
	Stephen Boyd <swboyd@chromium.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Rob Clark <robdclark@chromium.org>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk
Date: Tue, 30 Mar 2021 06:19:28 +0300	[thread overview]
Message-ID: <YGKYwJf/7kWlaoDD@pendragon.ideasonboard.com> (raw)
In-Reply-To: <CAD=FV=UOk-PUREc-UOPqUDuhPAEUsBfx0LOAQHd9KkLAhpr7Tg@mail.gmail.com>

Hi Doug,

On Mon, Mar 29, 2021 at 07:57:05PM -0700, Doug Anderson wrote:
> On Tue, Mar 16, 2021 at 5:44 PM Doug Anderson wrote:
> > On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart wrote:
> > > 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?
> 
> OK, I've got something that maybe looks better. You can tell me what
> you think [1]. I did manage to use PM Runtime to avoid some of the
> complexity and I put that usage in simple-panel. We'll see if I get
> yelled at for adding more to simple-panel. ;-P
> 
> [1] https://lore.kernel.org/dri-devel/20210330025345.3980086-1-dianders@chromium.org/

Nice :-)

I'm unfortunately afraid I'm quite busy these days. Could you ping me in
a few weeks if I haven't reviewed the series ?

-- 
Regards,

Laurent Pinchart

WARNING: multiple messages have this Message-ID
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Doug Anderson <dianders@chromium.org>
Cc: Rob Clark <robdclark@chromium.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Jonas Karlman <jonas@kwiboo.se>, David Airlie <airlied@linux.ie>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	LKML <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk
Date: Tue, 30 Mar 2021 06:19:28 +0300	[thread overview]
Message-ID: <YGKYwJf/7kWlaoDD@pendragon.ideasonboard.com> (raw)
In-Reply-To: <CAD=FV=UOk-PUREc-UOPqUDuhPAEUsBfx0LOAQHd9KkLAhpr7Tg@mail.gmail.com>

Hi Doug,

On Mon, Mar 29, 2021 at 07:57:05PM -0700, Doug Anderson wrote:
> On Tue, Mar 16, 2021 at 5:44 PM Doug Anderson wrote:
> > On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart wrote:
> > > 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?
> 
> OK, I've got something that maybe looks better. You can tell me what
> you think [1]. I did manage to use PM Runtime to avoid some of the
> complexity and I put that usage in simple-panel. We'll see if I get
> yelled at for adding more to simple-panel. ;-P
> 
> [1] https://lore.kernel.org/dri-devel/20210330025345.3980086-1-dianders@chromium.org/

Nice :-)

I'm unfortunately afraid I'm quite busy these days. Could you ping me in
a few weeks if I haven't reviewed the series ?

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-03-30  3:20 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-04 23:51 [PATCH 1/3] drm/bridge: ti-sn65dsi86: Simplify refclk handling Douglas Anderson
2021-03-04 23:51 ` Douglas Anderson
2021-03-04 23:52 ` [PATCH 2/3] drm/bridge: ti-sn65dsi86: Move code in prep for EDID read fix Douglas Anderson
2021-03-04 23:52   ` Douglas Anderson
2021-03-05 10:07   ` Robert Foss
2021-03-05 10:07     ` Robert Foss
2021-03-12  2:50   ` Bjorn Andersson
2021-03-12  2:50     ` Bjorn Andersson
2021-03-13 20:25   ` Stephen Boyd
2021-03-13 20:25     ` Stephen Boyd
2021-03-13 21:12   ` Laurent Pinchart
2021-03-13 21:12     ` Laurent Pinchart
2021-03-15 16:31     ` Doug Anderson
2021-03-15 16:31       ` Doug Anderson
2021-03-15 16:41       ` Laurent Pinchart
2021-03-15 16:41         ` Laurent Pinchart
2021-03-04 23:52 ` [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk Douglas Anderson
2021-03-04 23:52   ` Douglas Anderson
2021-03-12  2:59   ` Bjorn Andersson
2021-03-12  2:59     ` Bjorn Andersson
2021-03-13 21:16   ` Laurent Pinchart
2021-03-13 21:16     ` Laurent Pinchart
2021-03-15 16:25     ` Doug Anderson
2021-03-15 16:25       ` Doug Anderson
2021-03-16 21:46       ` Laurent Pinchart
2021-03-16 21:46         ` Laurent Pinchart
2021-03-17  0:44         ` Doug Anderson
2021-03-17  0:44           ` Doug Anderson
2021-03-30  2:57           ` Doug Anderson
2021-03-30  2:57             ` Doug Anderson
2021-03-30  3:19             ` Laurent Pinchart [this message]
2021-03-30  3:19               ` Laurent Pinchart
2021-03-05 10:00 ` [PATCH 1/3] drm/bridge: ti-sn65dsi86: Simplify refclk handling Robert Foss
2021-03-05 10:00   ` Robert Foss
2021-03-12  2:49 ` Bjorn Andersson
2021-03-12  2:49   ` Bjorn Andersson
2021-03-13 20:23 ` Stephen Boyd
2021-03-13 20:23   ` Stephen Boyd
2021-03-13 21:02 ` Laurent Pinchart
2021-03-13 21:02   ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YGKYwJf/7kWlaoDD@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=narmstrong@baylibre.com \
    --cc=robdclark@chromium.org \
    --cc=sam@ravnborg.org \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.