[PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

[PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

[Pin-yen Lin]

Skip the drm_bridge_chain_pre_enable call when the bridge is already
pre_enabled. This make pre_enable and post_disable (thus
pm_runtime_get/put) symmetric.

Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 4b361d7d5e44..08de501c436e 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 	 * EDID, for this chip, we need to do a full poweron, otherwise it will
 	 * fail.
 	 */
-	drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
+	if (poweroff)
+		drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
 
 	edid = drm_get_edid(connector,
 			    ps_bridge->page[PAGE0_DP_CNTL]->adapter);
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 2/2] drm/bridge: ps8640: Add a cache for EDID

[Pin-yen Lin]

When there are multiple EDID reads, the bridge will be repeatedly
enabled and disabled. Add a cache for EDID to speed this up.

Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 61 ++++++++++++++++----------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 08de501c436e..4d594be8b89c 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -105,6 +105,7 @@ struct ps8640 {
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_powerdown;
 	struct device_link *link;
+	struct edid *edid;
 	bool pre_enabled;
 	bool need_post_hpd_delay;
 };
@@ -543,34 +544,37 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 	bool poweroff = !ps_bridge->pre_enabled;
-	struct edid *edid;
 
-	/*
-	 * When we end calling get_edid() triggered by an ioctl, i.e
-	 *
-	 *   drm_mode_getconnector (ioctl)
-	 *     -> drm_helper_probe_single_connector_modes
-	 *        -> drm_bridge_connector_get_modes
-	 *           -> ps8640_bridge_get_edid
-	 *
-	 * We need to make sure that what we need is enabled before reading
-	 * EDID, for this chip, we need to do a full poweron, otherwise it will
-	 * fail.
-	 */
-	if (poweroff)
-		drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
+	if (!ps_bridge->edid) {
+		/*
+		 * When we end calling get_edid() triggered by an ioctl, i.e
+		 *
+		 *   drm_mode_getconnector (ioctl)
+		 *     -> drm_helper_probe_single_connector_modes
+		 *        -> drm_bridge_connector_get_modes
+		 *           -> ps8640_bridge_get_edid
+		 *
+		 * We need to make sure that what we need is enabled before
+		 * reading EDID, for this chip, we need to do a full poweron,
+		 * otherwise it will fail.
+		 */
+		if (poweroff)
+			drm_atomic_bridge_chain_pre_enable(bridge,
+							   connector->state->state);
 
-	edid = drm_get_edid(connector,
-			    ps_bridge->page[PAGE0_DP_CNTL]->adapter);
+		ps_bridge->edid = drm_get_edid(connector,
+					       ps_bridge->page[PAGE0_DP_CNTL]->adapter);
 
-	/*
-	 * If we call the get_edid() function without having enabled the chip
-	 * before, return the chip to its original power state.
-	 */
-	if (poweroff)
-		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
+		/*
+		 * If we call the get_edid() function without having enabled the
+		 * chip before, return the chip to its original power state.
+		 */
+		if (poweroff)
+			drm_atomic_bridge_chain_post_disable(bridge,
+							     connector->state->state);
+	}
 
-	return edid;
+	return drm_edid_duplicate(ps_bridge->edid);
 }
 
 static void ps8640_runtime_disable(void *data)
@@ -767,6 +771,14 @@ static int ps8640_probe(struct i2c_client *client)
 	return ret;
 }
 
+static void ps8640_remove(struct i2c_client *client)
+{
+	struct ps8640 *ps_bridge = i2c_get_clientdata(client);
+
+	kfree(ps_bridge->edid);
+	ps_bridge->edid = NULL;
+}
+
 static const struct of_device_id ps8640_match[] = {
 	{ .compatible = "parade,ps8640" },
 	{ }
@@ -775,6 +787,7 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
 
 static struct i2c_driver ps8640_driver = {
 	.probe_new = ps8640_probe,
+	.remove = ps8640_remove,
 	.driver = {
 		.name = "ps8640",
 		.of_match_table = ps8640_match,
-- 
2.40.0.rc1.284.g88254d51c5-goog

Re: [PATCH 2/2] drm/bridge: ps8640: Add a cache for EDID

[Robert Foss]

On Tue, Mar 14, 2023 at 12:01 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> When there are multiple EDID reads, the bridge will be repeatedly
> enabled and disabled. Add a cache for EDID to speed this up.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/parade-ps8640.c | 61 ++++++++++++++++----------
>  1 file changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 08de501c436e..4d594be8b89c 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -105,6 +105,7 @@ struct ps8640 {
>         struct gpio_desc *gpio_reset;
>         struct gpio_desc *gpio_powerdown;
>         struct device_link *link;
> +       struct edid *edid;
>         bool pre_enabled;
>         bool need_post_hpd_delay;
>  };
> @@ -543,34 +544,37 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>  {
>         struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>         bool poweroff = !ps_bridge->pre_enabled;
> -       struct edid *edid;
>
> -       /*
> -        * When we end calling get_edid() triggered by an ioctl, i.e
> -        *
> -        *   drm_mode_getconnector (ioctl)
> -        *     -> drm_helper_probe_single_connector_modes
> -        *        -> drm_bridge_connector_get_modes
> -        *           -> ps8640_bridge_get_edid
> -        *
> -        * We need to make sure that what we need is enabled before reading
> -        * EDID, for this chip, we need to do a full poweron, otherwise it will
> -        * fail.
> -        */
> -       if (poweroff)
> -               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> +       if (!ps_bridge->edid) {
> +               /*
> +                * When we end calling get_edid() triggered by an ioctl, i.e
> +                *
> +                *   drm_mode_getconnector (ioctl)
> +                *     -> drm_helper_probe_single_connector_modes
> +                *        -> drm_bridge_connector_get_modes
> +                *           -> ps8640_bridge_get_edid
> +                *
> +                * We need to make sure that what we need is enabled before
> +                * reading EDID, for this chip, we need to do a full poweron,
> +                * otherwise it will fail.
> +                */
> +               if (poweroff)
> +                       drm_atomic_bridge_chain_pre_enable(bridge,
> +                                                          connector->state->state);
>
> -       edid = drm_get_edid(connector,
> -                           ps_bridge->page[PAGE0_DP_CNTL]->adapter);
> +               ps_bridge->edid = drm_get_edid(connector,
> +                                              ps_bridge->page[PAGE0_DP_CNTL]->adapter);
>
> -       /*
> -        * If we call the get_edid() function without having enabled the chip
> -        * before, return the chip to its original power state.
> -        */
> -       if (poweroff)
> -               drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
> +               /*
> +                * If we call the get_edid() function without having enabled the
> +                * chip before, return the chip to its original power state.
> +                */
> +               if (poweroff)
> +                       drm_atomic_bridge_chain_post_disable(bridge,
> +                                                            connector->state->state);
> +       }
>
> -       return edid;
> +       return drm_edid_duplicate(ps_bridge->edid);
>  }
>
>  static void ps8640_runtime_disable(void *data)
> @@ -767,6 +771,14 @@ static int ps8640_probe(struct i2c_client *client)
>         return ret;
>  }
>
> +static void ps8640_remove(struct i2c_client *client)
> +{
> +       struct ps8640 *ps_bridge = i2c_get_clientdata(client);
> +
> +       kfree(ps_bridge->edid);
> +       ps_bridge->edid = NULL;
> +}
> +
>  static const struct of_device_id ps8640_match[] = {
>         { .compatible = "parade,ps8640" },
>         { }
> @@ -775,6 +787,7 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
>
>  static struct i2c_driver ps8640_driver = {
>         .probe_new = ps8640_probe,
> +       .remove = ps8640_remove,
>         .driver = {
>                 .name = "ps8640",
>                 .of_match_table = ps8640_match,
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>


Reviewed-by: Robert Foss <rfoss@kernel.org>

Re: [PATCH 2/2] drm/bridge: ps8640: Add a cache for EDID

[Doug Anderson]

Hi,

On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> When there are multiple EDID reads, the bridge will be repeatedly
> enabled and disabled. Add a cache for EDID to speed this up.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/parade-ps8640.c | 61 ++++++++++++++++----------
>  1 file changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 08de501c436e..4d594be8b89c 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -105,6 +105,7 @@ struct ps8640 {
>         struct gpio_desc *gpio_reset;
>         struct gpio_desc *gpio_powerdown;
>         struct device_link *link;
> +       struct edid *edid;
>         bool pre_enabled;
>         bool need_post_hpd_delay;
>  };
> @@ -543,34 +544,37 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>  {
>         struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>         bool poweroff = !ps_bridge->pre_enabled;
> -       struct edid *edid;
>
> -       /*
> -        * When we end calling get_edid() triggered by an ioctl, i.e
> -        *
> -        *   drm_mode_getconnector (ioctl)
> -        *     -> drm_helper_probe_single_connector_modes
> -        *        -> drm_bridge_connector_get_modes
> -        *           -> ps8640_bridge_get_edid
> -        *
> -        * We need to make sure that what we need is enabled before reading
> -        * EDID, for this chip, we need to do a full poweron, otherwise it will
> -        * fail.
> -        */
> -       if (poweroff)
> -               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> +       if (!ps_bridge->edid) {
> +               /*
> +                * When we end calling get_edid() triggered by an ioctl, i.e
> +                *
> +                *   drm_mode_getconnector (ioctl)
> +                *     -> drm_helper_probe_single_connector_modes
> +                *        -> drm_bridge_connector_get_modes
> +                *           -> ps8640_bridge_get_edid
> +                *
> +                * We need to make sure that what we need is enabled before
> +                * reading EDID, for this chip, we need to do a full poweron,
> +                * otherwise it will fail.
> +                */
> +               if (poweroff)
> +                       drm_atomic_bridge_chain_pre_enable(bridge,
> +                                                          connector->state->state);
>
> -       edid = drm_get_edid(connector,
> -                           ps_bridge->page[PAGE0_DP_CNTL]->adapter);
> +               ps_bridge->edid = drm_get_edid(connector,
> +                                              ps_bridge->page[PAGE0_DP_CNTL]->adapter);
>
> -       /*
> -        * If we call the get_edid() function without having enabled the chip
> -        * before, return the chip to its original power state.
> -        */
> -       if (poweroff)
> -               drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
> +               /*
> +                * If we call the get_edid() function without having enabled the
> +                * chip before, return the chip to its original power state.
> +                */
> +               if (poweroff)
> +                       drm_atomic_bridge_chain_post_disable(bridge,
> +                                                            connector->state->state);
> +       }
>
> -       return edid;
> +       return drm_edid_duplicate(ps_bridge->edid);
>  }
>
>  static void ps8640_runtime_disable(void *data)
> @@ -767,6 +771,14 @@ static int ps8640_probe(struct i2c_client *client)
>         return ret;
>  }
>
> +static void ps8640_remove(struct i2c_client *client)
> +{
> +       struct ps8640 *ps_bridge = i2c_get_clientdata(client);
> +
> +       kfree(ps_bridge->edid);
> +       ps_bridge->edid = NULL;

nit: no need to clear this to NULL since the driver is exiting.

Other than the small nit:

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

Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

[Robert Foss]

On Tue, Mar 14, 2023 at 12:01 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> Skip the drm_bridge_chain_pre_enable call when the bridge is already
> pre_enabled. This make pre_enable and post_disable (thus
> pm_runtime_get/put) symmetric.
>
> Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 4b361d7d5e44..08de501c436e 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>          * EDID, for this chip, we need to do a full poweron, otherwise it will
>          * fail.
>          */
> -       drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> +       if (poweroff)
> +               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
>
>         edid = drm_get_edid(connector,
>                             ps_bridge->page[PAGE0_DP_CNTL]->adapter);
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>

Reviewed-by: Robert Foss <rfoss@kernel.org>

Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

[Doug Anderson]

Hi,

On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> Skip the drm_bridge_chain_pre_enable call when the bridge is already
> pre_enabled. This make pre_enable and post_disable (thus
> pm_runtime_get/put) symmetric.
>
> Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 4b361d7d5e44..08de501c436e 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>          * EDID, for this chip, we need to do a full poweron, otherwise it will
>          * fail.
>          */
> -       drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> +       if (poweroff)
> +               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);

It always seemed weird to me that this function was asymmetric, so I
like this change, thanks!

I also remember wondering before how this function was safe, though.
The callpath for getting here from the ioctl is documented in the
function and when I look at it I wonder if anything is preventing the
bridge from being enabled / disabled through normal means at the same
time your function is running. That could cause all sorts of badness
if it is indeed possible. Does anyone reading this know if that's
indeed a problem?

I suppose that, if this is unsafe, it's no more unsafe now than it was
before your patch, so I guess:

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

If there are no issues, I'll plan to land this patch and the next one
to drm-misc-next sometime late-ish next week.

Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

[Pin-yen Lin]

Hi Doug,

On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote:
> >
> > Skip the drm_bridge_chain_pre_enable call when the bridge is already
> > pre_enabled. This make pre_enable and post_disable (thus
> > pm_runtime_get/put) symmetric.
> >
> > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > ---
> >
> >  drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 4b361d7d5e44..08de501c436e 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> >          * EDID, for this chip, we need to do a full poweron, otherwise it will
> >          * fail.
> >          */
> > -       drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> > +       if (poweroff)
> > +               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
>
> It always seemed weird to me that this function was asymmetric, so I
> like this change, thanks!
>
> I also remember wondering before how this function was safe, though.
> The callpath for getting here from the ioctl is documented in the
> function and when I look at it I wonder if anything is preventing the
> bridge from being enabled / disabled through normal means at the same
> time your function is running. That could cause all sorts of badness
> if it is indeed possible. Does anyone reading this know if that's
> indeed a problem?

If the "normal mean" is disabling the bridge, then we are probably
disabling the whole display pipeline. If so, is the EDID still
relevant in this case?

drm_get_edid returns NULL whenever error happens, and the helpers seem
to handle this case properly.
>
> I suppose that, if this is unsafe, it's no more unsafe now than it was
> before your patch, so I guess:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> If there are no issues, I'll plan to land this patch and the next one
> to drm-misc-next sometime late-ish next week.

Thanks for the review. I'll submit a v2 to collect the review tags and
fix up the nit in patch 2/2.

Best regards,
Pin-yen

Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

[Doug Anderson]

Hi,

On Tue, Mar 14, 2023 at 8:28 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> Hi Doug,
>
> On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote:
> > >
> > > Skip the drm_bridge_chain_pre_enable call when the bridge is already
> > > pre_enabled. This make pre_enable and post_disable (thus
> > > pm_runtime_get/put) symmetric.
> > >
> > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
> > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > ---
> > >
> > >  drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > index 4b361d7d5e44..08de501c436e 100644
> > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> > >          * EDID, for this chip, we need to do a full poweron, otherwise it will
> > >          * fail.
> > >          */
> > > -       drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> > > +       if (poweroff)
> > > +               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> >
> > It always seemed weird to me that this function was asymmetric, so I
> > like this change, thanks!
> >
> > I also remember wondering before how this function was safe, though.
> > The callpath for getting here from the ioctl is documented in the
> > function and when I look at it I wonder if anything is preventing the
> > bridge from being enabled / disabled through normal means at the same
> > time your function is running. That could cause all sorts of badness
> > if it is indeed possible. Does anyone reading this know if that's
> > indeed a problem?
>
> If the "normal mean" is disabling the bridge, then we are probably
> disabling the whole display pipeline. If so, is the EDID still
> relevant in this case?

In general when we do a "modeset" I believe that the display pipeline
is disabled and re-enabled. On a Chromebook test image you can see
this disable / re-enable happen when you switch between "VT2" and the
main login screen.

If the display pipeline is disabled / re-enabled then it should still
be fine to keep the EDID cached, so that's not what I'm worried about.
I'm more worried that someone could be querying the EDID at the same
time that someone else was turning the screen off. In that case it
would be possible for "poweroff" to be true (because the screen was on
when we started reading the EDID) and then partway through the screen
could get turned off.

-Doug

Re: [PATCH 1/2] drm/bridge: ps8640: Skip redundant bridge enable

[Pin-yen Lin]

Hi,

On Thu, Mar 16, 2023 at 5:34 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Mar 14, 2023 at 8:28 PM Pin-yen Lin <treapking@chromium.org> wrote:
> >
> > Hi Doug,
> >
> > On Wed, Mar 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote:
> > > >
> > > > Skip the drm_bridge_chain_pre_enable call when the bridge is already
> > > > pre_enabled. This make pre_enable and post_disable (thus
> > > > pm_runtime_get/put) symmetric.
> > > >
> > > > Fixes: 46f206304db0 ("drm/bridge: ps8640: Rework power state handling")
> > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > > > ---
> > > >
> > > >  drivers/gpu/drm/bridge/parade-ps8640.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > index 4b361d7d5e44..08de501c436e 100644
> > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > @@ -557,7 +557,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> > > >          * EDID, for this chip, we need to do a full poweron, otherwise it will
> > > >          * fail.
> > > >          */
> > > > -       drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> > > > +       if (poweroff)
> > > > +               drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> > >
> > > It always seemed weird to me that this function was asymmetric, so I
> > > like this change, thanks!
> > >
> > > I also remember wondering before how this function was safe, though.
> > > The callpath for getting here from the ioctl is documented in the
> > > function and when I look at it I wonder if anything is preventing the
> > > bridge from being enabled / disabled through normal means at the same
> > > time your function is running. That could cause all sorts of badness
> > > if it is indeed possible. Does anyone reading this know if that's
> > > indeed a problem?
> >
> > If the "normal mean" is disabling the bridge, then we are probably
> > disabling the whole display pipeline. If so, is the EDID still
> > relevant in this case?
>
> In general when we do a "modeset" I believe that the display pipeline
> is disabled and re-enabled. On a Chromebook test image you can see
> this disable / re-enable happen when you switch between "VT2" and the
> main login screen.
>
> If the display pipeline is disabled / re-enabled then it should still
> be fine to keep the EDID cached, so that's not what I'm worried about.
> I'm more worried that someone could be querying the EDID at the same
> time that someone else was turning the screen off. In that case it
> would be possible for "poweroff" to be true (because the screen was on

You mean "poweroff" to be "false", right? That is,
"ps_bridge->pre_enabled" is true. So the .get_edid function assumes
that the pipeline is enabled, but another thread is turning off the
screen.

> when we started reading the EDID) and then partway through the screen
> could get turned off.

Thanks for the detailed explanation. In that case, we probably get an
error and return a NULL EDID. But do we need the EDID when the screen
is turned off? And the EDID should be re-read if the screen is turned
back on.

However, in a reversed setting, if the .get_edid is reading EDID when
the pipeline is disabled (poweroff=true), but someone enables the
pipeline in between. In that case, .get_edid might disable the bridge
and panel after the pipeline is enabled.

Anyway, the function is not safe, but it's no more unsafe than before.
Patch 2/2 should lower the chance for anything bad to happen by adding
a cache by only read EDID once.
>
> -Doug

Thanks and regards,
Pin-yen