[PATCH v3 00/12] drm: Fix EDID reading on ti-sn65dsi86

[PATCH v3 00/12] drm: Fix EDID reading on ti-sn65dsi86

[Douglas Anderson]

The primary goal of this series is to try to properly fix EDID reading
for eDP panels using the ti-sn65dsi86 bridge.

Previously we had a patch that added EDID reading but it turned out
not to work at bootup. This caused some extra churn at bootup as we
tried (and failed) to read the EDID several times and also ended up
forcing us to use the hardcoded mode at boot. With this patch series I
believe EDID reading is reliable at boot now and we never use the
hardcoded mode.

This series is the logical successor to the 3-part series containing
the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only
if refclk") [1] though only one actual patch is the same between the
two.

This series starts out with some general / obvious fixes and moves on
to some more specific and maybe controversial ones. I wouldn't object
to some of the earlier ones landing if they look ready.

This patch was developed against drm-misc-next on a
sc7180-trogdor-lazor device. To get things booting for me, I had to
use Stephen's patch [2] to keep from crashing but otherwise all the
patches I needed were here.

Primary change between v2 and v3 is to stop doing the EDID caching in
the core. I also added Andrzej's review tags.

[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
[2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com/

Changes in v3:
- Removed "NOTES" from commit message.
- Rebased now that we're not moving EDID caching to the core.
- Separating out patch to block AUX channel when not powered.
- Added note about boot speed implications.
- ("Fail aux transfers right away if not powered") split out for v3.

Changes in v2:
- Removed 2nd paragraph in commit message.

Douglas Anderson (12):
  drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
  drm/bridge: ti-sn65dsi86: Simplify refclk handling
  drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment
  drm/bridge: ti-sn65dsi86: Reorder remove()
  drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable()
  drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function
  drm/bridge: ti-sn65dsi86: Remove extra call:
    drm_connector_update_edid_property()
  drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
  drm/bridge: ti-sn65dsi86: Fail aux transfers right away if not powered
  drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided
  drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes
  drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare /
    prepare

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 97 ++++++++++++++++-----------
 drivers/gpu/drm/drm_bridge.c          |  3 +
 drivers/gpu/drm/panel/Kconfig         |  1 +
 drivers/gpu/drm/panel/panel-simple.c  | 93 +++++++++++++++++++------
 4 files changed, 134 insertions(+), 60 deletions(-)

-- 
2.31.0.208.g409f899ff0-goog

[PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()

[Douglas Anderson]

The drm_bridge_chain_pre_enable() is not the proper opposite of
drm_bridge_chain_post_disable(). It continues along the chain to
_before_ the starting bridge. Let's fix that.

Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

(no changes since v1)

 drivers/gpu/drm/drm_bridge.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 64f0effb52ac..044acd07c153 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
 		if (iter->funcs->pre_enable)
 			iter->funcs->pre_enable(iter);
+
+		if (iter == bridge)
+			break;
 	}
 }
 EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> The drm_bridge_chain_pre_enable() is not the proper opposite of
> drm_bridge_chain_post_disable(). It continues along the chain to
> _before_ the starting bridge. Let's fix that.
> 
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/drm_bridge.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..044acd07c153 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>  	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>  		if (iter->funcs->pre_enable)
>  			iter->funcs->pre_enable(iter);
> +
> +		if (iter == bridge)
> +			break;

This looks good as it matches drm_atomic_bridge_chain_disable().

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'm curious though, given that the bridge passed to the function should
be the one closest to the encoder, does this make a difference ?

>  	}
>  }
>  EXPORT_SYMBOL(drm_bridge_chain_pre_enable);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()

[Doug Anderson]

Hi,

On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > drm_bridge_chain_post_disable(). It continues along the chain to
> > _before_ the starting bridge. Let's fix that.
> >
> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/gpu/drm/drm_bridge.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 64f0effb52ac..044acd07c153 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> >       list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >               if (iter->funcs->pre_enable)
> >                       iter->funcs->pre_enable(iter);
> > +
> > +             if (iter == bridge)
> > +                     break;
>
> This looks good as it matches drm_atomic_bridge_chain_disable().
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks for your review here and several of the other patches. Can you
suggest any plan for getting them landed? It would at least be nice to
get the non-controversial ones landed.


> I'm curious though, given that the bridge passed to the function should
> be the one closest to the encoder, does this make a difference ?

Yes, that's how I discovered it originally. Let's see. So if I don't
have this patch but have the rest of the series then I get a splat at
bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
in the chain than my bridge chip. Here's the splat:

 msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22
 ------------[ cut here ]------------
 disp_cc_mdss_ahb_clk status stuck at 'off'
 WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92
clk_branch_toggle+0x194/0x280
 Modules linked in: joydev
 CPU: 7 PID: 404 Comm: frecon Tainted: G    B             5.12.0-rc3-lockdep+ #2
 Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
 pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
 pc : clk_branch_toggle+0x194/0x280
 lr : clk_branch_toggle+0x190/0x280
 ...
 Call trace:
  clk_branch_toggle+0x194/0x280
  clk_branch2_enable+0x28/0x34
  clk_core_enable+0x2f4/0x6b4
  clk_enable+0x54/0x74
  dsi_phy_enable_resource+0x80/0xd8
  msm_dsi_phy_enable+0xa8/0x4a8
  enable_phy+0x9c/0xf4
  dsi_mgr_bridge_pre_enable+0x23c/0x4b0
  drm_bridge_chain_pre_enable+0xac/0xe4
  ti_sn_bridge_connector_get_modes+0x134/0x1b8
  drm_helper_probe_single_connector_modes+0x49c/0x1358
  drm_mode_getconnector+0x460/0xe98
  drm_ioctl_kernel+0x144/0x228
  drm_ioctl+0x418/0x7cc
  drm_compat_ioctl+0x1bc/0x230
  __arm64_compat_sys_ioctl+0x14c/0x188
  el0_svc_common+0x128/0x23c
  do_el0_svc_compat+0x50/0x60
  el0_svc_compat+0x24/0x34
  el0_sync_compat_handler+0xc0/0xf0
  el0_sync_compat+0x174/0x180

Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()

[Laurent Pinchart]

Hi Doug,

On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
> On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
> > On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > > drm_bridge_chain_post_disable(). It continues along the chain to
> > > _before_ the starting bridge. Let's fix that.
> > >
> > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  drivers/gpu/drm/drm_bridge.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 64f0effb52ac..044acd07c153 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> > >       list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > >               if (iter->funcs->pre_enable)
> > >                       iter->funcs->pre_enable(iter);
> > > +
> > > +             if (iter == bridge)
> > > +                     break;
> >
> > This looks good as it matches drm_atomic_bridge_chain_disable().
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks for your review here and several of the other patches. Can you
> suggest any plan for getting them landed? It would at least be nice to
> get the non-controversial ones landed.

Do you have commit access to drm-misc ? If not, given your
contributions, I think you qualify for it.

> > I'm curious though, given that the bridge passed to the function should
> > be the one closest to the encoder, does this make a difference ?
> 
> Yes, that's how I discovered it originally. Let's see. So if I don't
> have this patch but have the rest of the series then I get a splat at
> bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
> in the chain than my bridge chip. Here's the splat:

Right, I think it's caused by a later patch in the series calling this
function with a different bridge than the one closest to the encoder.

>  msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22
>  ------------[ cut here ]------------
>  disp_cc_mdss_ahb_clk status stuck at 'off'
>  WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92
> clk_branch_toggle+0x194/0x280
>  Modules linked in: joydev
>  CPU: 7 PID: 404 Comm: frecon Tainted: G    B             5.12.0-rc3-lockdep+ #2
>  Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>  pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
>  pc : clk_branch_toggle+0x194/0x280
>  lr : clk_branch_toggle+0x190/0x280
>  ...
>  Call trace:
>   clk_branch_toggle+0x194/0x280
>   clk_branch2_enable+0x28/0x34
>   clk_core_enable+0x2f4/0x6b4
>   clk_enable+0x54/0x74
>   dsi_phy_enable_resource+0x80/0xd8
>   msm_dsi_phy_enable+0xa8/0x4a8
>   enable_phy+0x9c/0xf4
>   dsi_mgr_bridge_pre_enable+0x23c/0x4b0
>   drm_bridge_chain_pre_enable+0xac/0xe4
>   ti_sn_bridge_connector_get_modes+0x134/0x1b8
>   drm_helper_probe_single_connector_modes+0x49c/0x1358
>   drm_mode_getconnector+0x460/0xe98
>   drm_ioctl_kernel+0x144/0x228
>   drm_ioctl+0x418/0x7cc
>   drm_compat_ioctl+0x1bc/0x230
>   __arm64_compat_sys_ioctl+0x14c/0x188
>   el0_svc_common+0x128/0x23c
>   do_el0_svc_compat+0x50/0x60
>   el0_svc_compat+0x24/0x34
>   el0_sync_compat_handler+0xc0/0xf0
>   el0_sync_compat+0x174/0x180

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()

[Doug Anderson]

Hi,

On Wed, Apr 14, 2021 at 6:56 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Doug,
>
> On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
> > On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
> > > On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > > > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > > > drm_bridge_chain_post_disable(). It continues along the chain to
> > > > _before_ the starting bridge. Let's fix that.
> > > >
> > > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  drivers/gpu/drm/drm_bridge.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > > index 64f0effb52ac..044acd07c153 100644
> > > > --- a/drivers/gpu/drm/drm_bridge.c
> > > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> > > >       list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > > >               if (iter->funcs->pre_enable)
> > > >                       iter->funcs->pre_enable(iter);
> > > > +
> > > > +             if (iter == bridge)
> > > > +                     break;
> > >
> > > This looks good as it matches drm_atomic_bridge_chain_disable().
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Thanks for your review here and several of the other patches. Can you
> > suggest any plan for getting them landed? It would at least be nice to
> > get the non-controversial ones landed.
>
> Do you have commit access to drm-misc ? If not, given your
> contributions, I think you qualify for it.

No, I don't have access. I searched for how to get it and read through
the qualifications and, you're right, I think I do. I've hopefully
followed the right flow and created an issue to give me ssh access:

https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/348

Is that something you (or someone else on this CC list) approves?


> > > I'm curious though, given that the bridge passed to the function should
> > > be the one closest to the encoder, does this make a difference ?
> >
> > Yes, that's how I discovered it originally. Let's see. So if I don't
> > have this patch but have the rest of the series then I get a splat at
> > bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
> > in the chain than my bridge chip. Here's the splat:
>
> Right, I think it's caused by a later patch in the series calling this
> function with a different bridge than the one closest to the encoder.

Yup! I still wanted this patch to be first in the series, though,
since it's a bugfix that we'd want to land even if the later patches
changed in some way.

-Doug

[PATCH v3 02/12] drm/bridge: ti-sn65dsi86: Simplify refclk handling

[Douglas Anderson]

The clock framework makes it simple to deal with an optional clock.
You can call clk_get_optional() and if the clock isn't specified it'll
just return NULL without complaint. It's valid to pass NULL to
enable/disable/prepare/unprepare. Let's make use of this to simplify
things a tiny bit.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Robert Foss <robert.foss@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

(no changes since v2)

Changes in v2:
- Removed 2nd paragraph in commit message.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 88df4dd0f39d..96fe8f2c0ea9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1275,14 +1275,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	pdata->refclk = devm_clk_get(pdata->dev, "refclk");
-	if (IS_ERR(pdata->refclk)) {
-		ret = PTR_ERR(pdata->refclk);
-		if (ret == -EPROBE_DEFER)
-			return ret;
-		DRM_DEBUG_KMS("refclk not found\n");
-		pdata->refclk = NULL;
-	}
+	pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk");
+	if (IS_ERR(pdata->refclk))
+		return PTR_ERR(pdata->refclk);
 
 	ret = ti_sn_bridge_parse_dsi_host(pdata);
 	if (ret)
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v3 03/12] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment

[Douglas Anderson]

A random comment inside a function had "/**" in front of it. That
doesn't make sense. Remove.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 96fe8f2c0ea9..76f43af6735d 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -788,7 +788,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	/* set dsi clk frequency value */
 	ti_sn_bridge_set_dsi_rate(pdata);
 
-	/**
+	/*
 	 * The SN65DSI86 only supports ASSR Display Authentication method and
 	 * this method is enabled by default. An eDP panel must support this
 	 * authentication method. We need to enable this method in the eDP panel
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 03/12] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:37PM -0700, Douglas Anderson wrote:
> A random comment inside a function had "/**" in front of it. That
> doesn't make sense. Remove.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 96fe8f2c0ea9..76f43af6735d 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -788,7 +788,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>  	/* set dsi clk frequency value */
>  	ti_sn_bridge_set_dsi_rate(pdata);
>  
> -	/**
> +	/*
>  	 * The SN65DSI86 only supports ASSR Display Authentication method and
>  	 * this method is enabled by default. An eDP panel must support this
>  	 * authentication method. We need to enable this method in the eDP panel

-- 
Regards,

Laurent Pinchart

[PATCH v3 04/12] drm/bridge: ti-sn65dsi86: Reorder remove()

[Douglas Anderson]

Let's make the remove() function strictly the reverse of the probe()
function so it's easier to reason about.

This patch was created by code inspection and should move us closer to
a proper remove.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

Changes in v3:
- Removed "NOTES" from commit message.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 76f43af6735d..c006678c9921 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
 	if (!pdata)
 		return -EINVAL;
 
-	kfree(pdata->edid);
-	ti_sn_debugfs_remove(pdata);
-
-	of_node_put(pdata->host_node);
-
-	pm_runtime_disable(pdata->dev);
-
 	if (pdata->dsi) {
 		mipi_dsi_detach(pdata->dsi);
 		mipi_dsi_device_unregister(pdata->dsi);
 	}
 
+	kfree(pdata->edid);
+
+	ti_sn_debugfs_remove(pdata);
+
 	drm_bridge_remove(&pdata->bridge);
 
+	pm_runtime_disable(pdata->dev);
+
+	of_node_put(pdata->host_node);
+
 	return 0;
 }
 
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 04/12] drm/bridge: ti-sn65dsi86: Reorder remove()

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:38PM -0700, Douglas Anderson wrote:
> Let's make the remove() function strictly the reverse of the probe()
> function so it's easier to reason about.
> 
> This patch was created by code inspection and should move us closer to
> a proper remove.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> Changes in v3:
> - Removed "NOTES" from commit message.
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 76f43af6735d..c006678c9921 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
>  	if (!pdata)
>  		return -EINVAL;
>  
> -	kfree(pdata->edid);
> -	ti_sn_debugfs_remove(pdata);
> -
> -	of_node_put(pdata->host_node);
> -
> -	pm_runtime_disable(pdata->dev);
> -
>  	if (pdata->dsi) {
>  		mipi_dsi_detach(pdata->dsi);
>  		mipi_dsi_device_unregister(pdata->dsi);
>  	}
>  
> +	kfree(pdata->edid);
> +
> +	ti_sn_debugfs_remove(pdata);
> +
>  	drm_bridge_remove(&pdata->bridge);
>  
> +	pm_runtime_disable(pdata->dev);
> +
> +	of_node_put(pdata->host_node);
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

[PATCH v3 05/12] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable()

[Douglas Anderson]

We prepared the panel in pre_enable() so we should unprepare it in
post_disable() to match.

This becomes important once we start using pre_enable() and
post_disable() to make sure things are powered on (and then off again)
when reading the EDID.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index c006678c9921..e30460002c48 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -452,8 +452,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
 	/* disable DP PLL */
 	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
-
-	drm_panel_unprepare(pdata->panel);
 }
 
 static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
@@ -869,6 +867,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
 
+	drm_panel_unprepare(pdata->panel);
+
 	clk_disable_unprepare(pdata->refclk);
 
 	pm_runtime_put_sync(pdata->dev);
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 05/12] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable()

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:39PM -0700, Douglas Anderson wrote:
> We prepared the panel in pre_enable() so we should unprepare it in
> post_disable() to match.
> 
> This becomes important once we start using pre_enable() and
> post_disable() to make sure things are powered on (and then off again)
> when reading the EDID.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c006678c9921..e30460002c48 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -452,8 +452,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
>  	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
>  	/* disable DP PLL */
>  	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
> -
> -	drm_panel_unprepare(pdata->panel);
>  }
>  
>  static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> @@ -869,6 +867,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
>  {
>  	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>  
> +	drm_panel_unprepare(pdata->panel);
> +
>  	clk_disable_unprepare(pdata->refclk);
>  
>  	pm_runtime_put_sync(pdata->dev);

-- 
Regards,

Laurent Pinchart

[PATCH v3 06/12] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function

[Douglas Anderson]

If we just leave the detect() function as NULL then the upper layers
assume we're always connected. There's no reason for a stub.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index e30460002c48..51db30d573c1 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -306,20 +306,8 @@ static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
 	.mode_valid = ti_sn_bridge_connector_mode_valid,
 };
 
-static enum drm_connector_status
-ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
-{
-	/**
-	 * TODO: Currently if drm_panel is present, then always
-	 * return the status as connected. Need to add support to detect
-	 * device state for hot pluggable scenarios.
-	 */
-	return connector_status_connected;
-}
-
 static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = ti_sn_bridge_connector_detect,
 	.destroy = drm_connector_cleanup,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 06/12] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:40PM -0700, Douglas Anderson wrote:
> If we just leave the detect() function as NULL then the upper layers
> assume we're always connected. There's no reason for a stub.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index e30460002c48..51db30d573c1 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -306,20 +306,8 @@ static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
>  	.mode_valid = ti_sn_bridge_connector_mode_valid,
>  };
>  
> -static enum drm_connector_status
> -ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
> -{
> -	/**
> -	 * TODO: Currently if drm_panel is present, then always
> -	 * return the status as connected. Need to add support to detect
> -	 * device state for hot pluggable scenarios.
> -	 */
> -	return connector_status_connected;
> -}
> -
>  static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.detect = ti_sn_bridge_connector_detect,
>  	.destroy = drm_connector_cleanup,
>  	.reset = drm_atomic_helper_connector_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,

-- 
Regards,

Laurent Pinchart

[PATCH v3 07/12] drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property()

[Douglas Anderson]

As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
drm_connector") the drm_get_edid() function calls
drm_connector_update_edid_property() for us. There's no reason for us
to call it again.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 51db30d573c1..6390bc58f29a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -270,7 +270,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
 	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
 	struct edid *edid = pdata->edid;
-	int num, ret;
+	int num;
 
 	if (!edid) {
 		pm_runtime_get_sync(pdata->dev);
@@ -279,12 +279,9 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 	}
 
 	if (edid && drm_edid_is_valid(edid)) {
-		ret = drm_connector_update_edid_property(connector, edid);
-		if (!ret) {
-			num = drm_add_edid_modes(connector, edid);
-			if (num)
-				return num;
-		}
+		num = drm_add_edid_modes(connector, edid);
+		if (num)
+			return num;
 	}
 
 	return drm_panel_get_modes(pdata->panel, connector);
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 07/12] drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property()

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:41PM -0700, Douglas Anderson wrote:
> As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
> drm_connector") the drm_get_edid() function calls
> drm_connector_update_edid_property() for us. There's no reason for us
> to call it again.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This looks like a widespread issue, would you be able to send a patch to
address all the other drivers ?

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 51db30d573c1..6390bc58f29a 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -270,7 +270,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
>  	struct edid *edid = pdata->edid;
> -	int num, ret;
> +	int num;
>  
>  	if (!edid) {
>  		pm_runtime_get_sync(pdata->dev);
> @@ -279,12 +279,9 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
>  	}
>  
>  	if (edid && drm_edid_is_valid(edid)) {
> -		ret = drm_connector_update_edid_property(connector, edid);
> -		if (!ret) {
> -			num = drm_add_edid_modes(connector, edid);
> -			if (num)
> -				return num;
> -		}
> +		num = drm_add_edid_modes(connector, edid);
> +		if (num)
> +			return num;
>  	}
>  
>  	return drm_panel_get_modes(pdata->panel, connector);

-- 
Regards,

Laurent Pinchart

[PATCH v3 08/12] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID

[Douglas Anderson]

eDP panels won't provide their EDID unless they're powered on. Let's
chain a power-on before we read the EDID. This roughly matches what
was done in 'parade-ps8640.c'.

NOTE: The old code attempted to call pm_runtime_get_sync() before
reading the EDID. While that was enough to power the bridge chip on,
it wasn't enough to talk to the panel for two reasons:
1. Since we never ran the bridge chip's pre-enable then we never set
   the bit to ignore HPD. This meant the bridge chip didn't even _try_
   to go out on the bus and communicate with the panel.
2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
   if the panel wasn't on.

ALSO NOTE: Without the future patch ("drm/panel: panel-simple: Use
runtime pm to avoid excessive unprepare / prepare") there will be boot
speed implications here. Specifically we'll power the panel on to read
the EDID, then fully off. Then we'll likely have to wait the minimum
time between power off and power on.

Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Rebased now that we're not moving EDID caching to the core.
- Separating out patch to block AUX channel when not powered.
- Added note about boot speed implications.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 6390bc58f29a..543590801a8e 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -129,6 +129,7 @@
  * @dp_lanes:     Count of dp_lanes we're using.
  * @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:  If true then pre_enable() has run.
  *
  * @gchip:        If we expose our GPIOs, this is used.
  * @gchip_output: A cache of whether we've set GPIOs to output.  This
@@ -157,6 +158,7 @@ struct ti_sn_bridge {
 	int				dp_lanes;
 	u8				ln_assign;
 	u8				ln_polrs;
+	bool				pre_enabled;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -270,12 +272,17 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
 	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
 	struct edid *edid = pdata->edid;
+	bool was_enabled;
 	int num;
 
 	if (!edid) {
-		pm_runtime_get_sync(pdata->dev);
+		was_enabled = pdata->pre_enabled;
+
+		if (!was_enabled)
+			drm_bridge_chain_pre_enable(&pdata->bridge);
 		edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
-		pm_runtime_put(pdata->dev);
+		if (!was_enabled)
+			drm_bridge_chain_post_disable(&pdata->bridge);
 	}
 
 	if (edid && drm_edid_is_valid(edid)) {
@@ -846,12 +853,16 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 			   HPD_DISABLE);
 
 	drm_panel_prepare(pdata->panel);
+
+	pdata->pre_enabled = true;
 }
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
 
+	pdata->pre_enabled = false;
+
 	drm_panel_unprepare(pdata->panel);
 
 	clk_disable_unprepare(pdata->refclk);
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v3 09/12] drm/bridge: ti-sn65dsi86: Fail aux transfers right away if not powered

[Douglas Anderson]

If the bridge (and panel) haven't been powered on then AUX transfers
just won't work. Let's just fail them instantly.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
If the patch ("drm/panel: panel-simple: Use runtime pm to avoid
excessive unprepare / prepare") is accepted then we could consider
actually powering the panel on instead of failing the
transfer. However, without that patch the overhead would just be too
much since we need to do several AUX transfers for a single EDID read
and powering up and down each time would just be too much.

Changes in v3:
- ("Fail aux transfers right away if not powered") split out for v3.

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 543590801a8e..a76cac93cb46 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -896,6 +896,13 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
 	int ret;
 	u8 addr_len[SN_AUX_LENGTH_REG + 1 - SN_AUX_ADDR_19_16_REG];
 
+	/*
+	 * Things just won't work if the panel isn't powered. Return failure
+	 * right away.
+	 */
+	if (!pdata->pre_enabled)
+		return -EIO;
+
 	if (len > SN_AUX_MAX_PAYLOAD_BYTES)
 		return -EINVAL;
 
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v3 10/12] drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided

[Douglas Anderson]

Though I don't have access to any hardware that uses ti-sn65dsi86 and
_doesn't_ provide a "refclk", I believe that we'll have trouble
reading the EDID at bootup in that case. Specifically I believe that
if there's no "refclk" we need the MIPI source clock to be active
before we can successfully read the EDID. My evidence here is that, in
testing, I couldn't read the EDID until I turned on the DPPLL in the
bridge chip and that the DPPLL needs the input clock to be active.

Since this is hard to support, let's punt trying to read the EDID if
there's no "refclk".

I don't believe there are any users of the ti-sn65dsi86 bridge chip
that _don't_ use "refclk". The bridge chip is _very_ inflexible in
that mode. The only time I've seen that mode used was for some really
early prototype hardware that was thrown in the e-waste bin years ago
when we realized how inflexible it was.

Even if someone is using the bridge chip without the "refclk" they're
in no worse shape than they were before the (fairly recent) commit
58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index a76cac93cb46..fb50f9f95b0f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -275,6 +275,18 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 	bool was_enabled;
 	int num;
 
+	/*
+	 * Don't try to read the EDID if no refclk. In theory it is possible
+	 * to make this work but it's tricky. I believe that we need to get
+	 * our upstream MIPI source to provide a pixel clock before we can
+	 * do AUX transations but we need to be able to read the EDID before
+	 * we've picked a display mode. The bridge is already super limited
+	 * if you try to use it without a refclk so presumably limiting to
+	 * the fixed modes our downstream panel reports is fine.
+	 */
+	if (!pdata->refclk)
+		goto exit;
+
 	if (!edid) {
 		was_enabled = pdata->pre_enabled;
 
@@ -291,6 +303,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 			return num;
 	}
 
+exit:
 	return drm_panel_get_modes(pdata->panel, connector);
 }
 
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 10/12] drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:44PM -0700, Douglas Anderson wrote:
> Though I don't have access to any hardware that uses ti-sn65dsi86 and
> _doesn't_ provide a "refclk", I believe that we'll have trouble
> reading the EDID at bootup in that case. Specifically I believe that
> if there's no "refclk" we need the MIPI source clock to be active
> before we can successfully read the EDID. My evidence here is that, in
> testing, I couldn't read the EDID until I turned on the DPPLL in the
> bridge chip and that the DPPLL needs the input clock to be active.
> 
> Since this is hard to support, let's punt trying to read the EDID if
> there's no "refclk".
> 
> I don't believe there are any users of the ti-sn65dsi86 bridge chip
> that _don't_ use "refclk". The bridge chip is _very_ inflexible in
> that mode. The only time I've seen that mode used was for some really
> early prototype hardware that was thrown in the e-waste bin years ago
> when we realized how inflexible it was.
> 
> Even if someone is using the bridge chip without the "refclk" they're
> in no worse shape than they were before the (fairly recent) commit
> 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index a76cac93cb46..fb50f9f95b0f 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -275,6 +275,18 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
>  	bool was_enabled;
>  	int num;
>  
> +	/*
> +	 * Don't try to read the EDID if no refclk. In theory it is possible
> +	 * to make this work but it's tricky. I believe that we need to get
> +	 * our upstream MIPI source to provide a pixel clock before we can
> +	 * do AUX transations but we need to be able to read the EDID before
> +	 * we've picked a display mode. The bridge is already super limited
> +	 * if you try to use it without a refclk so presumably limiting to
> +	 * the fixed modes our downstream panel reports is fine.
> +	 */
> +	if (!pdata->refclk)
> +		goto exit;
> +
>  	if (!edid) {
>  		was_enabled = pdata->pre_enabled;
>  
> @@ -291,6 +303,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
>  			return num;
>  	}
>  
> +exit:
>  	return drm_panel_get_modes(pdata->panel, connector);
>  }
>  

-- 
Regards,

Laurent Pinchart

[PATCH v3 11/12] drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes

[Douglas Anderson]

Now that we can properly read the EDID for modes there should be no
reason to fallback to the fixed modes that our downstream panel driver
provides us. Let's make that clear by:
- Putting an error message in the logs if we fall back.
- Putting a comment in saying what's going on.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index fb50f9f95b0f..3b61898cf9cb 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -303,6 +303,13 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 			return num;
 	}
 
+	/*
+	 * Ideally this should never happen and we could remove the fallback
+	 * but let's preserve old behavior.
+	 */
+	DRM_DEV_ERROR(pdata->dev,
+		      "Failed to read EDID; falling back to panel modes");
+
 exit:
 	return drm_panel_get_modes(pdata->panel, connector);
 }
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 11/12] drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:45PM -0700, Douglas Anderson wrote:
> Now that we can properly read the EDID for modes there should be no
> reason to fallback to the fixed modes that our downstream panel driver
> provides us. Let's make that clear by:
> - Putting an error message in the logs if we fall back.
> - Putting a comment in saying what's going on.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index fb50f9f95b0f..3b61898cf9cb 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -303,6 +303,13 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
>  			return num;
>  	}
>  
> +	/*
> +	 * Ideally this should never happen and we could remove the fallback
> +	 * but let's preserve old behavior.
> +	 */
> +	DRM_DEV_ERROR(pdata->dev,
> +		      "Failed to read EDID; falling back to panel modes");
> +
>  exit:
>  	return drm_panel_get_modes(pdata->panel, connector);
>  }

-- 
Regards,

Laurent Pinchart

[PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare

[Douglas Anderson]

Unpreparing and re-preparing a panel can be a really heavy
operation. Panels datasheets often specify something on the order of
500ms as the delay you should insert after turning off the panel
before turning it on again. In addition, turning on a panel can have
delays on the order of 100ms - 200ms before the panel will assert HPD
(AKA "panel ready"). The above means that we should avoid turning a
panel off if we're going to turn it on again shortly.

The above becomes a problem when we want to read the EDID of a
panel. The way that ordering works is that userspace wants to read the
EDID of the panel _before_ fully enabling it so that it can set the
initial mode correctly. However, we can't read the EDID until we power
it up. This leads to code that does this dance (like
ps8640_bridge_get_edid()):

1. When userspace requests EDID / the panel modes (through an ioctl),
   we power on the panel just enough to read the EDID and then power
   it off.
2. Userspace then turns the panel on.

There's likely not much time between step #1 and #2 and so we want to
avoid powering the panel off and on again between those two steps.

Let's use Runtime PM to help us. We'll move the existing prepare() and
unprepare() to be runtime resume() and runtime suspend(). Now when we
want to prepare() or unprepare() we just increment or decrement the
refcount. We'll default to a 1 second autosuspend delay which seems
sane given the typical delays we see for panels.

A few notes:
- It seems the existing unprepare() and prepare() are defined to be
  no-ops if called extra times. We'll preserve that behavior.
- This is a slight change in the ABI of simple panel. If something was
  absolutely relying on the unprepare() to happen instantly that
  simply won't be the case anymore. I'm not aware of anyone relying on
  that behavior, but if there is someone then we'll need to figure out
  how to enable (or disable) this new delayed behavior selectively.
- In order for this to work we now have a hard dependency on
  "PM". From memory this is a legit thing to assume these days and we
  don't have to find some fallback to keep working if someone wants to
  build their system without "PM".

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/panel/Kconfig        |  1 +
 drivers/gpu/drm/panel/panel-simple.c | 93 +++++++++++++++++++++-------
 2 files changed, 73 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 4894913936e9..ef87d92cdf49 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -80,6 +80,7 @@ config DRM_PANEL_SIMPLE
 	tristate "support for simple panels"
 	depends on OF
 	depends on BACKLIGHT_CLASS_DEVICE
+	depends on PM
 	select VIDEOMODE_HELPERS
 	help
 	  DRM panel driver for dumb panels that need at most a regulator and
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index be312b5c04dd..6b22872b3281 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -27,6 +27,7 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 #include <video/display_timing.h>
@@ -175,6 +176,8 @@ struct panel_simple {
 	bool enabled;
 	bool no_hpd;
 
+	bool prepared;
+
 	ktime_t prepared_time;
 	ktime_t unprepared_time;
 
@@ -334,19 +337,31 @@ static int panel_simple_disable(struct drm_panel *panel)
 	return 0;
 }
 
+static int panel_simple_suspend(struct device *dev)
+{
+	struct panel_simple *p = dev_get_drvdata(dev);
+
+	gpiod_set_value_cansleep(p->enable_gpio, 0);
+	regulator_disable(p->supply);
+	p->unprepared_time = ktime_get();
+
+	return 0;
+}
+
 static int panel_simple_unprepare(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
+	int ret;
 
-	if (p->prepared_time == 0)
+	/* Unpreparing when already unprepared is a no-op */
+	if (!p->prepared)
 		return 0;
 
-	gpiod_set_value_cansleep(p->enable_gpio, 0);
-
-	regulator_disable(p->supply);
-
-	p->prepared_time = 0;
-	p->unprepared_time = ktime_get();
+	pm_runtime_mark_last_busy(panel->dev);
+	ret = pm_runtime_put_autosuspend(panel->dev);
+	if (ret < 0)
+		return ret;
+	p->prepared = false;
 
 	return 0;
 }
@@ -376,22 +391,19 @@ static int panel_simple_get_hpd_gpio(struct device *dev,
 	return 0;
 }
 
-static int panel_simple_prepare_once(struct drm_panel *panel)
+static int panel_simple_prepare_once(struct panel_simple *p)
 {
-	struct panel_simple *p = to_panel_simple(panel);
+	struct device *dev = p->base.dev;
 	unsigned int delay;
 	int err;
 	int hpd_asserted;
 	unsigned long hpd_wait_us;
 
-	if (p->prepared_time != 0)
-		return 0;
-
 	panel_simple_wait(p->unprepared_time, p->desc->delay.unprepare);
 
 	err = regulator_enable(p->supply);
 	if (err < 0) {
-		dev_err(panel->dev, "failed to enable supply: %d\n", err);
+		dev_err(dev, "failed to enable supply: %d\n", err);
 		return err;
 	}
 
@@ -405,7 +417,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
 
 	if (p->hpd_gpio) {
 		if (IS_ERR(p->hpd_gpio)) {
-			err = panel_simple_get_hpd_gpio(panel->dev, p, false);
+			err = panel_simple_get_hpd_gpio(dev, p, false);
 			if (err)
 				goto error;
 		}
@@ -423,7 +435,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
 
 		if (err) {
 			if (err != -ETIMEDOUT)
-				dev_err(panel->dev,
+				dev_err(dev,
 					"error waiting for hpd GPIO: %d\n", err);
 			goto error;
 		}
@@ -447,25 +459,46 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
  */
 #define MAX_PANEL_PREPARE_TRIES		5
 
-static int panel_simple_prepare(struct drm_panel *panel)
+static int panel_simple_resume(struct device *dev)
 {
+	struct panel_simple *p = dev_get_drvdata(dev);
 	int ret;
 	int try;
 
 	for (try = 0; try < MAX_PANEL_PREPARE_TRIES; try++) {
-		ret = panel_simple_prepare_once(panel);
+		ret = panel_simple_prepare_once(p);
 		if (ret != -ETIMEDOUT)
 			break;
 	}
 
 	if (ret == -ETIMEDOUT)
-		dev_err(panel->dev, "Prepare timeout after %d tries\n", try);
+		dev_err(dev, "Prepare timeout after %d tries\n", try);
 	else if (try)
-		dev_warn(panel->dev, "Prepare needed %d retries\n", try);
+		dev_warn(dev, "Prepare needed %d retries\n", try);
 
 	return ret;
 }
 
+static int panel_simple_prepare(struct drm_panel *panel)
+{
+	struct panel_simple *p = to_panel_simple(panel);
+	int ret;
+
+	/* Preparing when already prepared is a no-op */
+	if (p->prepared)
+		return 0;
+
+	ret = pm_runtime_get_sync(panel->dev);
+	if (ret < 0) {
+		pm_runtime_put_autosuspend(panel->dev);
+		return ret;
+	}
+
+	p->prepared = true;
+
+	return 0;
+}
+
 static int panel_simple_enable(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
@@ -748,6 +781,18 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 		break;
 	}
 
+	dev_set_drvdata(dev, panel);
+
+	/*
+	 * We use runtime PM for prepare / unprepare since those power the panel
+	 * on and off and those can be very slow operations. This is important
+	 * to optimize powering the panel on briefly to read the EDID before
+	 * fully enabling the panel.
+	 */
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
 	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
 
 	err = drm_panel_of_backlight(&panel->base);
@@ -756,8 +801,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 
 	drm_panel_add(&panel->base);
 
-	dev_set_drvdata(dev, panel);
-
 	return 0;
 
 free_ddc:
@@ -4603,10 +4646,17 @@ static void panel_simple_platform_shutdown(struct platform_device *pdev)
 	panel_simple_shutdown(&pdev->dev);
 }
 
+static const struct dev_pm_ops panel_simple_pm_ops = {
+	SET_RUNTIME_PM_OPS(panel_simple_suspend, panel_simple_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
 static struct platform_driver panel_simple_platform_driver = {
 	.driver = {
 		.name = "panel-simple",
 		.of_match_table = platform_of_match,
+		.pm = &panel_simple_pm_ops,
 	},
 	.probe = panel_simple_platform_probe,
 	.remove = panel_simple_platform_remove,
@@ -4901,6 +4951,7 @@ static struct mipi_dsi_driver panel_simple_dsi_driver = {
 	.driver = {
 		.name = "panel-simple-dsi",
 		.of_match_table = dsi_of_match,
+		.pm = &panel_simple_pm_ops,
 	},
 	.probe = panel_simple_dsi_probe,
 	.remove = panel_simple_dsi_remove,
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare

[Laurent Pinchart]

Hi Doug,

Thank you for the patch.

On Fri, Apr 02, 2021 at 03:28:46PM -0700, Douglas Anderson wrote:
> Unpreparing and re-preparing a panel can be a really heavy
> operation. Panels datasheets often specify something on the order of
> 500ms as the delay you should insert after turning off the panel
> before turning it on again. In addition, turning on a panel can have
> delays on the order of 100ms - 200ms before the panel will assert HPD
> (AKA "panel ready"). The above means that we should avoid turning a
> panel off if we're going to turn it on again shortly.
> 
> The above becomes a problem when we want to read the EDID of a
> panel. The way that ordering works is that userspace wants to read the
> EDID of the panel _before_ fully enabling it so that it can set the
> initial mode correctly. However, we can't read the EDID until we power
> it up. This leads to code that does this dance (like
> ps8640_bridge_get_edid()):
> 
> 1. When userspace requests EDID / the panel modes (through an ioctl),
>    we power on the panel just enough to read the EDID and then power
>    it off.
> 2. Userspace then turns the panel on.
> 
> There's likely not much time between step #1 and #2 and so we want to
> avoid powering the panel off and on again between those two steps.
> 
> Let's use Runtime PM to help us. We'll move the existing prepare() and
> unprepare() to be runtime resume() and runtime suspend(). Now when we
> want to prepare() or unprepare() we just increment or decrement the
> refcount. We'll default to a 1 second autosuspend delay which seems
> sane given the typical delays we see for panels.
> 
> A few notes:
> - It seems the existing unprepare() and prepare() are defined to be
>   no-ops if called extra times. We'll preserve that behavior.

The prepare and unprepare calls are supposed to be balanced, which
should allow us to drop this check. Do you have a reason to suspect that
it may not be the case ?

> - This is a slight change in the ABI of simple panel. If something was
>   absolutely relying on the unprepare() to happen instantly that
>   simply won't be the case anymore. I'm not aware of anyone relying on
>   that behavior, but if there is someone then we'll need to figure out
>   how to enable (or disable) this new delayed behavior selectively.
> - In order for this to work we now have a hard dependency on
>   "PM". From memory this is a legit thing to assume these days and we
>   don't have to find some fallback to keep working if someone wants to
>   build their system without "PM".

Sounds fine to me.

The code looks good to me. Possibly with the prepared check removed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/panel/Kconfig        |  1 +
>  drivers/gpu/drm/panel/panel-simple.c | 93 +++++++++++++++++++++-------
>  2 files changed, 73 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 4894913936e9..ef87d92cdf49 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -80,6 +80,7 @@ config DRM_PANEL_SIMPLE
>  	tristate "support for simple panels"
>  	depends on OF
>  	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on PM
>  	select VIDEOMODE_HELPERS
>  	help
>  	  DRM panel driver for dumb panels that need at most a regulator and
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index be312b5c04dd..6b22872b3281 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -27,6 +27,7 @@
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  
>  #include <video/display_timing.h>
> @@ -175,6 +176,8 @@ struct panel_simple {
>  	bool enabled;
>  	bool no_hpd;
>  
> +	bool prepared;
> +
>  	ktime_t prepared_time;
>  	ktime_t unprepared_time;
>  
> @@ -334,19 +337,31 @@ static int panel_simple_disable(struct drm_panel *panel)
>  	return 0;
>  }
>  
> +static int panel_simple_suspend(struct device *dev)
> +{
> +	struct panel_simple *p = dev_get_drvdata(dev);
> +
> +	gpiod_set_value_cansleep(p->enable_gpio, 0);
> +	regulator_disable(p->supply);
> +	p->unprepared_time = ktime_get();
> +
> +	return 0;
> +}
> +
>  static int panel_simple_unprepare(struct drm_panel *panel)
>  {
>  	struct panel_simple *p = to_panel_simple(panel);
> +	int ret;
>  
> -	if (p->prepared_time == 0)
> +	/* Unpreparing when already unprepared is a no-op */
> +	if (!p->prepared)
>  		return 0;
>  
> -	gpiod_set_value_cansleep(p->enable_gpio, 0);
> -
> -	regulator_disable(p->supply);
> -
> -	p->prepared_time = 0;
> -	p->unprepared_time = ktime_get();
> +	pm_runtime_mark_last_busy(panel->dev);
> +	ret = pm_runtime_put_autosuspend(panel->dev);
> +	if (ret < 0)
> +		return ret;
> +	p->prepared = false;
>  
>  	return 0;
>  }
> @@ -376,22 +391,19 @@ static int panel_simple_get_hpd_gpio(struct device *dev,
>  	return 0;
>  }
>  
> -static int panel_simple_prepare_once(struct drm_panel *panel)
> +static int panel_simple_prepare_once(struct panel_simple *p)
>  {
> -	struct panel_simple *p = to_panel_simple(panel);
> +	struct device *dev = p->base.dev;
>  	unsigned int delay;
>  	int err;
>  	int hpd_asserted;
>  	unsigned long hpd_wait_us;
>  
> -	if (p->prepared_time != 0)
> -		return 0;
> -
>  	panel_simple_wait(p->unprepared_time, p->desc->delay.unprepare);
>  
>  	err = regulator_enable(p->supply);
>  	if (err < 0) {
> -		dev_err(panel->dev, "failed to enable supply: %d\n", err);
> +		dev_err(dev, "failed to enable supply: %d\n", err);
>  		return err;
>  	}
>  
> @@ -405,7 +417,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
>  
>  	if (p->hpd_gpio) {
>  		if (IS_ERR(p->hpd_gpio)) {
> -			err = panel_simple_get_hpd_gpio(panel->dev, p, false);
> +			err = panel_simple_get_hpd_gpio(dev, p, false);
>  			if (err)
>  				goto error;
>  		}
> @@ -423,7 +435,7 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
>  
>  		if (err) {
>  			if (err != -ETIMEDOUT)
> -				dev_err(panel->dev,
> +				dev_err(dev,
>  					"error waiting for hpd GPIO: %d\n", err);
>  			goto error;
>  		}
> @@ -447,25 +459,46 @@ static int panel_simple_prepare_once(struct drm_panel *panel)
>   */
>  #define MAX_PANEL_PREPARE_TRIES		5
>  
> -static int panel_simple_prepare(struct drm_panel *panel)
> +static int panel_simple_resume(struct device *dev)
>  {
> +	struct panel_simple *p = dev_get_drvdata(dev);
>  	int ret;
>  	int try;
>  
>  	for (try = 0; try < MAX_PANEL_PREPARE_TRIES; try++) {
> -		ret = panel_simple_prepare_once(panel);
> +		ret = panel_simple_prepare_once(p);
>  		if (ret != -ETIMEDOUT)
>  			break;
>  	}
>  
>  	if (ret == -ETIMEDOUT)
> -		dev_err(panel->dev, "Prepare timeout after %d tries\n", try);
> +		dev_err(dev, "Prepare timeout after %d tries\n", try);
>  	else if (try)
> -		dev_warn(panel->dev, "Prepare needed %d retries\n", try);
> +		dev_warn(dev, "Prepare needed %d retries\n", try);
>  
>  	return ret;
>  }
>  
> +static int panel_simple_prepare(struct drm_panel *panel)
> +{
> +	struct panel_simple *p = to_panel_simple(panel);
> +	int ret;
> +
> +	/* Preparing when already prepared is a no-op */
> +	if (p->prepared)
> +		return 0;
> +
> +	ret = pm_runtime_get_sync(panel->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_autosuspend(panel->dev);
> +		return ret;
> +	}
> +
> +	p->prepared = true;
> +
> +	return 0;
> +}
> +
>  static int panel_simple_enable(struct drm_panel *panel)
>  {
>  	struct panel_simple *p = to_panel_simple(panel);
> @@ -748,6 +781,18 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  		break;
>  	}
>  
> +	dev_set_drvdata(dev, panel);
> +
> +	/*
> +	 * We use runtime PM for prepare / unprepare since those power the panel
> +	 * on and off and those can be very slow operations. This is important
> +	 * to optimize powering the panel on briefly to read the EDID before
> +	 * fully enabling the panel.
> +	 */
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
>  	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
>  
>  	err = drm_panel_of_backlight(&panel->base);
> @@ -756,8 +801,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  
>  	drm_panel_add(&panel->base);
>  
> -	dev_set_drvdata(dev, panel);
> -
>  	return 0;
>  
>  free_ddc:
> @@ -4603,10 +4646,17 @@ static void panel_simple_platform_shutdown(struct platform_device *pdev)
>  	panel_simple_shutdown(&pdev->dev);
>  }
>  
> +static const struct dev_pm_ops panel_simple_pm_ops = {
> +	SET_RUNTIME_PM_OPS(panel_simple_suspend, panel_simple_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +};
> +
>  static struct platform_driver panel_simple_platform_driver = {
>  	.driver = {
>  		.name = "panel-simple",
>  		.of_match_table = platform_of_match,
> +		.pm = &panel_simple_pm_ops,
>  	},
>  	.probe = panel_simple_platform_probe,
>  	.remove = panel_simple_platform_remove,
> @@ -4901,6 +4951,7 @@ static struct mipi_dsi_driver panel_simple_dsi_driver = {
>  	.driver = {
>  		.name = "panel-simple-dsi",
>  		.of_match_table = dsi_of_match,
> +		.pm = &panel_simple_pm_ops,
>  	},
>  	.probe = panel_simple_dsi_probe,
>  	.remove = panel_simple_dsi_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare

[Doug Anderson]

Hi,

On Wed, Apr 14, 2021 at 5:58 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> On Fri, Apr 02, 2021 at 03:28:46PM -0700, Douglas Anderson wrote:
> > Unpreparing and re-preparing a panel can be a really heavy
> > operation. Panels datasheets often specify something on the order of
> > 500ms as the delay you should insert after turning off the panel
> > before turning it on again. In addition, turning on a panel can have
> > delays on the order of 100ms - 200ms before the panel will assert HPD
> > (AKA "panel ready"). The above means that we should avoid turning a
> > panel off if we're going to turn it on again shortly.
> >
> > The above becomes a problem when we want to read the EDID of a
> > panel. The way that ordering works is that userspace wants to read the
> > EDID of the panel _before_ fully enabling it so that it can set the
> > initial mode correctly. However, we can't read the EDID until we power
> > it up. This leads to code that does this dance (like
> > ps8640_bridge_get_edid()):
> >
> > 1. When userspace requests EDID / the panel modes (through an ioctl),
> >    we power on the panel just enough to read the EDID and then power
> >    it off.
> > 2. Userspace then turns the panel on.
> >
> > There's likely not much time between step #1 and #2 and so we want to
> > avoid powering the panel off and on again between those two steps.
> >
> > Let's use Runtime PM to help us. We'll move the existing prepare() and
> > unprepare() to be runtime resume() and runtime suspend(). Now when we
> > want to prepare() or unprepare() we just increment or decrement the
> > refcount. We'll default to a 1 second autosuspend delay which seems
> > sane given the typical delays we see for panels.
> >
> > A few notes:
> > - It seems the existing unprepare() and prepare() are defined to be
> >   no-ops if called extra times. We'll preserve that behavior.
>
> The prepare and unprepare calls are supposed to be balanced, which
> should allow us to drop this check. Do you have a reason to suspect that
> it may not be the case ?

No, it was just code inspection. The old code definitely made an
effort to make enable of an already enabled panel a no-op and disable
of an already disabled panel a no-op. This is even before my
(somewhat) recent patch to make things timing based, though I did
touch the code.

Can I maybe suggest that getting rid of the extra check should be a
separate patch after this one? Then if it breaks someone it's easy to
just revert that one and we can still keep the runtime pm?


> > - This is a slight change in the ABI of simple panel. If something was
> >   absolutely relying on the unprepare() to happen instantly that
> >   simply won't be the case anymore. I'm not aware of anyone relying on
> >   that behavior, but if there is someone then we'll need to figure out
> >   how to enable (or disable) this new delayed behavior selectively.
> > - In order for this to work we now have a hard dependency on
> >   "PM". From memory this is a legit thing to assume these days and we
> >   don't have to find some fallback to keep working if someone wants to
> >   build their system without "PM".
>
> Sounds fine to me.
>
> The code looks good to me. Possibly with the prepared check removed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare

[Laurent Pinchart]

Hi Doug,

On Wed, Apr 14, 2021 at 06:22:57PM -0700, Doug Anderson wrote:
> On Wed, Apr 14, 2021 at 5:58 PM Laurent Pinchart wrote:
> > On Fri, Apr 02, 2021 at 03:28:46PM -0700, Douglas Anderson wrote:
> > > Unpreparing and re-preparing a panel can be a really heavy
> > > operation. Panels datasheets often specify something on the order of
> > > 500ms as the delay you should insert after turning off the panel
> > > before turning it on again. In addition, turning on a panel can have
> > > delays on the order of 100ms - 200ms before the panel will assert HPD
> > > (AKA "panel ready"). The above means that we should avoid turning a
> > > panel off if we're going to turn it on again shortly.
> > >
> > > The above becomes a problem when we want to read the EDID of a
> > > panel. The way that ordering works is that userspace wants to read the
> > > EDID of the panel _before_ fully enabling it so that it can set the
> > > initial mode correctly. However, we can't read the EDID until we power
> > > it up. This leads to code that does this dance (like
> > > ps8640_bridge_get_edid()):
> > >
> > > 1. When userspace requests EDID / the panel modes (through an ioctl),
> > >    we power on the panel just enough to read the EDID and then power
> > >    it off.
> > > 2. Userspace then turns the panel on.
> > >
> > > There's likely not much time between step #1 and #2 and so we want to
> > > avoid powering the panel off and on again between those two steps.
> > >
> > > Let's use Runtime PM to help us. We'll move the existing prepare() and
> > > unprepare() to be runtime resume() and runtime suspend(). Now when we
> > > want to prepare() or unprepare() we just increment or decrement the
> > > refcount. We'll default to a 1 second autosuspend delay which seems
> > > sane given the typical delays we see for panels.
> > >
> > > A few notes:
> > > - It seems the existing unprepare() and prepare() are defined to be
> > >   no-ops if called extra times. We'll preserve that behavior.
> >
> > The prepare and unprepare calls are supposed to be balanced, which
> > should allow us to drop this check. Do you have a reason to suspect that
> > it may not be the case ?
> 
> No, it was just code inspection. The old code definitely made an
> effort to make enable of an already enabled panel a no-op and disable
> of an already disabled panel a no-op. This is even before my
> (somewhat) recent patch to make things timing based, though I did
> touch the code.
> 
> Can I maybe suggest that getting rid of the extra check should be a
> separate patch after this one? Then if it breaks someone it's easy to
> just revert that one and we can still keep the runtime pm?

Sounds good to me.

> > > - This is a slight change in the ABI of simple panel. If something was
> > >   absolutely relying on the unprepare() to happen instantly that
> > >   simply won't be the case anymore. I'm not aware of anyone relying on
> > >   that behavior, but if there is someone then we'll need to figure out
> > >   how to enable (or disable) this new delayed behavior selectively.
> > > - In order for this to work we now have a hard dependency on
> > >   "PM". From memory this is a legit thing to assume these days and we
> > >   don't have to find some fallback to keep working if someone wants to
> > >   build their system without "PM".
> >
> > Sounds fine to me.
> >
> > The code looks good to me. Possibly with the prepared check removed,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart