diff mbox series

[v4,24/27] drm/panel: panel-simple: Cache the EDID as long as we retain power

Message ID 20210416153909.v4.24.If050957eaa85cf45b10bcf61e6f7fa61c9750ebf@changeid
State Superseded
Headers show
Series drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems | expand

Commit Message

Doug Anderson April 16, 2021, 10:39 p.m. UTC
It doesn't make sense to go out to the bus and read the EDID over and
over again. Let's cache it and throw away the cache when we turn power
off from the panel. Autosuspend means that even if there are several
calls to read the EDID before we officially turn the power on then we
should get good use out of this cache.

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

(no changes since v1)

 drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Bjorn Andersson April 23, 2021, 4:12 p.m. UTC | #1
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:

> It doesn't make sense to go out to the bus and read the EDID over and
> over again. Let's cache it and throw away the cache when we turn power
> off from the panel. Autosuspend means that even if there are several
> calls to read the EDID before we officially turn the power on then we
> should get good use out of this cache.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 40382c1be692..5a2953c4ca44 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -189,6 +189,8 @@ struct panel_simple {
>  	struct gpio_desc *enable_gpio;
>  	struct gpio_desc *hpd_gpio;
>  
> +	struct edid *edid;
> +
>  	struct drm_display_mode override_mode;
>  
>  	enum drm_panel_orientation orientation;
> @@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev)
>  	regulator_disable(p->supply);
>  	p->unprepared_time = ktime_get();
>  
> +	kfree(p->edid);
> +	p->edid = NULL;

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


But separate of this, shouldn't the driver have a pm_runtime_disable()
in the remove path to synchronize the autosleep? Or is that not how that
works?

Regards,
Bjorn

> +
>  	return 0;
>  }
>  
> @@ -510,15 +515,13 @@ static int panel_simple_get_modes(struct drm_panel *panel,
>  
>  	/* probe EDID if a DDC bus is available */
>  	if (p->ddc) {
> -		struct edid *edid;
> -
>  		pm_runtime_get_sync(panel->dev);
>  
> -		edid = drm_get_edid(connector, p->ddc);
> -		if (edid) {
> -			num += drm_add_edid_modes(connector, edid);
> -			kfree(edid);
> -		}
> +		if (!p->edid)
> +			p->edid = drm_get_edid(connector, p->ddc);
> +
> +		if (p->edid)
> +			num += drm_add_edid_modes(connector, p->edid);
>  
>  		pm_runtime_mark_last_busy(panel->dev);
>  		pm_runtime_put_autosuspend(panel->dev);
> -- 
> 2.31.1.368.gbe11c130af-goog
>
Doug Anderson April 23, 2021, 4:30 p.m. UTC | #2
Hi,

On Fri, Apr 23, 2021 at 9:12 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>

> On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote:

>

> > It doesn't make sense to go out to the bus and read the EDID over and

> > over again. Let's cache it and throw away the cache when we turn power

> > off from the panel. Autosuspend means that even if there are several

> > calls to read the EDID before we officially turn the power on then we

> > should get good use out of this cache.

> >

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

> > ---

> >

> > (no changes since v1)

> >

> >  drivers/gpu/drm/panel/panel-simple.c | 17 ++++++++++-------

> >  1 file changed, 10 insertions(+), 7 deletions(-)

> >

> > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c

> > index 40382c1be692..5a2953c4ca44 100644

> > --- a/drivers/gpu/drm/panel/panel-simple.c

> > +++ b/drivers/gpu/drm/panel/panel-simple.c

> > @@ -189,6 +189,8 @@ struct panel_simple {

> >       struct gpio_desc *enable_gpio;

> >       struct gpio_desc *hpd_gpio;

> >

> > +     struct edid *edid;

> > +

> >       struct drm_display_mode override_mode;

> >

> >       enum drm_panel_orientation orientation;

> > @@ -345,6 +347,9 @@ static int panel_simple_suspend(struct device *dev)

> >       regulator_disable(p->supply);

> >       p->unprepared_time = ktime_get();

> >

> > +     kfree(p->edid);

> > +     p->edid = NULL;

>

> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

>

>

> But separate of this, shouldn't the driver have a pm_runtime_disable()

> in the remove path to synchronize the autosleep? Or is that not how that

> works?


Indeed! I'll add a patch to the start of my v5 (coming shortly) that
fixes this. Thanks for catching!

-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 40382c1be692..5a2953c4ca44 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -189,6 +189,8 @@  struct panel_simple {
 	struct gpio_desc *enable_gpio;
 	struct gpio_desc *hpd_gpio;
 
+	struct edid *edid;
+
 	struct drm_display_mode override_mode;
 
 	enum drm_panel_orientation orientation;
@@ -345,6 +347,9 @@  static int panel_simple_suspend(struct device *dev)
 	regulator_disable(p->supply);
 	p->unprepared_time = ktime_get();
 
+	kfree(p->edid);
+	p->edid = NULL;
+
 	return 0;
 }
 
@@ -510,15 +515,13 @@  static int panel_simple_get_modes(struct drm_panel *panel,
 
 	/* probe EDID if a DDC bus is available */
 	if (p->ddc) {
-		struct edid *edid;
-
 		pm_runtime_get_sync(panel->dev);
 
-		edid = drm_get_edid(connector, p->ddc);
-		if (edid) {
-			num += drm_add_edid_modes(connector, edid);
-			kfree(edid);
-		}
+		if (!p->edid)
+			p->edid = drm_get_edid(connector, p->ddc);
+
+		if (p->edid)
+			num += drm_add_edid_modes(connector, p->edid);
 
 		pm_runtime_mark_last_busy(panel->dev);
 		pm_runtime_put_autosuspend(panel->dev);