| 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 |
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 >
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 --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);
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(-)