[v5,01/20] drm/panel: panel-simple: Add missing pm_runtime_disable() calls

In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
avoid excessive unprepare / prepare") we started using pm_runtime, but
my patch neglected to add the proper pm_runtime_disable(). Doh! Add
them now.

Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v5:
- Missing pm_runtime_disable() patch new for v5.

 drivers/gpu/drm/panel/panel-simple.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On Fri, Apr 23, 2021 at 12:59 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
> avoid excessive unprepare / prepare") we started using pm_runtime, but
> my patch neglected to add the proper pm_runtime_disable(). Doh! Add
> them now.
>
> Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v5:
> - Missing pm_runtime_disable() patch new for v5.
>
>  drivers/gpu/drm/panel/panel-simple.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 6b22872b3281..9746eda6f675 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -797,12 +797,14 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>
>         err = drm_panel_of_backlight(&panel->base);
>         if (err)
> -               goto free_ddc;
> +               goto disable_pm_runtime;
>
>         drm_panel_add(&panel->base);
>
>         return 0;
>
> +disable_pm_runtime:
> +       pm_runtime_disable(dev);
>  free_ddc:
>         if (panel->ddc)
>                 put_device(&panel->ddc->dev);
> @@ -818,6 +820,7 @@ static int panel_simple_remove(struct device *dev)
>         drm_panel_disable(&panel->base);
>         drm_panel_unprepare(&panel->base);
>
> +       pm_runtime_disable(dev);
>         if (panel->ddc)
>                 put_device(&panel->ddc->dev);
>
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson <dianders@chromium.org> wrote:

> In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
> avoid excessive unprepare / prepare") we started using pm_runtime, but
> my patch neglected to add the proper pm_runtime_disable(). Doh! Add
> them now.
>
> Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

This patch as such:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Notice however: you turn on pm runtime pm_runtime_enable()
in panel_simple_probe() but are you ever turning it off in
panel_simple_remove()?

I think pm_runtime_disable(); need to be added there?

Yours,
Linus Walleij

Hi,

On Thu, Apr 29, 2021 at 5:58 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Apr 23, 2021 at 6:59 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> > In commit 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to
> > avoid excessive unprepare / prepare") we started using pm_runtime, but
> > my patch neglected to add the proper pm_runtime_disable(). Doh! Add
> > them now.
> >
> > Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare")
> > Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> This patch as such:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Notice however: you turn on pm runtime pm_runtime_enable()
> in panel_simple_probe() but are you ever turning it off in
> panel_simple_remove()?
>
> I think pm_runtime_disable(); need to be added there?

I'm a bit confused. You're saying that I need to add
pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do
that? This patch adds two calls to pm_runtime_disable(). One of those
is in the probe error path and the other one is in
panel_simple_remove().

-Doug

On Fri, Apr 30, 2021 at 3:25 AM Doug Anderson <dianders@chromium.org> wrote:

> > I think pm_runtime_disable(); need to be added there?
>
> I'm a bit confused. You're saying that I need to add
> pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do
> that?

It does, sorry, too late at night :D

I was looking at the previous patch and mixed up which was the
patch and the patch to the patch...

Thanks, apply this!
Linus Walleij

Hi,

On Thu, Apr 29, 2021 at 6:28 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Apr 30, 2021 at 3:25 AM Doug Anderson <dianders@chromium.org> wrote:
>
> > > I think pm_runtime_disable(); need to be added there?
> >
> > I'm a bit confused. You're saying that I need to add
> > pm_runtime_disable() to panel_simple_remove()? Doesn't this patch do
> > that?
>
> It does, sorry, too late at night :D
>
> I was looking at the previous patch and mixed up which was the
> patch and the patch to the patch...
>
> Thanks, apply this!

Pushed this one patch. Rest of the series is pending adult
supervision. Overall summary:

1. I could probably push some of the early sn65dsi86 cleanup patches
in this series since they have Bjorn's review and are pretty much
no-ops / simple cleanups, but there's probably not tons gained for
shoving those in early.

2. The whole concept of breaking up the patch into sub-drivers has no
official Reviewed-by tags yet. Presumably Bjorn will give those a
re-review when he has time again. Assuming nobody is really upset
about it, I could land those which might unblock some of Bjorn's
future PWM work. It would probably be good to get an extra set of eyes
on them, though, just so someone else agrees that they're not "too
hacky" or anything. IMO it's actually a pretty nice solution, but I'm
biased!

3. Laurent and I had a big discussion on #dri-devel yesterday about
the EDID reading. He's not totally convinced with the idea of doing
this in the panel when the bridge could just do it by itself, but it
sounded like he might be coming around. Right now this is waiting on
Laurent to have time to get back to this.

My summary of the IRC discussion with Laurent (please correct if I got
this wrong):

a) In general I argued that it was important to be able to provide the
EDID and the DDC bus to the panel driver. Providing the EDID to the
panel driver allows the panel driver is one of the prerequisites for
my proposal for solving the "panel second sourcing" problem [1]. Being
able to provide the DDC bus to the panel will likely be important in
the eventual solution to Rajeev's problem [2].

b) Today, if we provide the DDC bus to simple-panel then simple-panel
will assume it's in charge of reading the EDID.

c) Having the panel driver involved in reading the EDID feels like it
makes sense to me. The panel driver knows how to power the panel on
enough to read the EDID. It also might know extra quirks needed to
read the EDID on a given panel. This feels a little cleaner (to me)
than just re-using the panel's "prepare" and assuming that a prepared
panel was ready for EDID read, though I can see that both may have
their advantages.

d) Laurent proposed that some eDP controllers might have special ways
to read an EDID but might not be able to provide a DDC bus or an i2c
bus. If we run into controllers like this then we would be painted
into a corner and we'd have to come up with a new solution. This is
definitely a good point, though it remains to be seen if this is
common with eDP (like Laurent says it is for HDMI). Some eDP panels
need custom DDC commands in order to be configured and so hopefully
all eDP bridges out there at least provide a DDC bus. It does feel
like this could be solved later, though. My patch series is leveraging
the existing concept that the panel driver is in charge of reading the
EDID if it's given the DDC bus, so it's not creating a new mechanism
but instead just continuing to use the existing mechanism. If the
existing mechanism doesn't work then it can be improved when there is
a need.

e) Laurent worried about circular dependencies and wanted to see my
solution to the problem before deciding if it was too big of a hack.
Hopefully it looks OK since it solves not only this problem but also
the HPD GPIO problem and will be important for when Bjorn exports the
PWM from the bridge chip.

[1] https://lore.kernel.org/lkml/CAD=FV=VZYOMPwQZzWdhJGh5cjJWw_EcM-wQVEivZ-bdGXjPrEQ@mail.gmail.com/
[2] https://lore.kernel.org/r/78c4bd291bd4a17ae2a1d02d0217de43@codeaurora.org

OK, I'll shut up now. ;-)

-Doug

Hi Doug,

On Fri, Apr 30, 2021 at 11:04 PM Doug Anderson <dianders@chromium.org> wrote:

> Pushed this one patch. Rest of the series is pending adult
> supervision. Overall summary:
>
> 1. I could probably push some of the early sn65dsi86 cleanup patches
> in this series since they have Bjorn's review and are pretty much
> no-ops / simple cleanups, but there's probably not tons gained for
> shoving those in early.

Those look good to me as well. I'd say just apply them.

To me it looks like up until and including patch 18?
Feel free to add my
Acked-by: Linus Walleij <linus.walleij@linaro.org>

On these.

> 2. The whole concept of breaking up the patch into sub-drivers has no
> official Reviewed-by tags yet. Presumably Bjorn will give those a
> re-review when he has time again.

It looks good to me so I sent an explicit ACK on that patch.

> 3. Laurent and I had a big discussion on #dri-devel yesterday about
> the EDID reading. He's not totally convinced with the idea of doing
> this in the panel when the bridge could just do it by itself, but it
> sounded like he might be coming around. Right now this is waiting on
> Laurent to have time to get back to this.

I dare not speak of this. Laurent has the long and tedious experience
with panels and pretty much anything related so if Laurent is
hesitant then I am hesitant too. His buy-in is absolutely required.

But IIUC that is just for patch 19+20?

It'd be good to apply the rest and get down the stack.

Just to keep you busy and make sure you don't run out of work
(haha) I noticed that the gpio_chip in this driver can use
the new GPIO_REGMAP helper library with the fixes just
landed in Torvald's tree.

At your convenience and when you think there is too little
stuff in your sn65dsi86 TODO, check out
pinctrl-bcm63xx.c for an example of select GPIO_REGMAP
made very simple (this works fine as long as they are bit
offsets starting from 0).

Yours,
Linus Walleij

Hi,

On Sat, May 1, 2021 at 5:07 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Doug,
>
> On Fri, Apr 30, 2021 at 11:04 PM Doug Anderson <dianders@chromium.org> wrote:
>
> > Pushed this one patch. Rest of the series is pending adult
> > supervision. Overall summary:
> >
> > 1. I could probably push some of the early sn65dsi86 cleanup patches
> > in this series since they have Bjorn's review and are pretty much
> > no-ops / simple cleanups, but there's probably not tons gained for
> > shoving those in early.
>
> Those look good to me as well. I'd say just apply them.
>
> To me it looks like up until and including patch 18?
> Feel free to add my
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> On these.

OK, thanks! I've just pushed these patches to drm-misc-next with your Ack:

63358e24ee79 drm/panel: panel-simple: Cache the EDID as long as we retain power
31e25395d8b7 drm/panel: panel-simple: Power the panel when reading the EDID
4318ea406e02 drm/panel: panel-simple: Remove extra call:
drm_connector_update_edid_property()
b137406d9679 drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen
w/out pre-enable
f7a5ee2cd3e2 drm/bridge: ti-sn65dsi86: Code motion of refclk
management functions
9bede63127c6 drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend
5c4381eeb709 drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code
bf73537f411b drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP
bridge into sub-drivers
bef236a5206c drm/bridge: ti-sn65dsi86: Move all the chip-related init
to the start
f94eb8a32863 drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata
3636fc25f760 drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe
52d54819c8ae drm/bridge: ti-sn65dsi86: Clean debugfs code
dea2500a820c drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable
905d66d08d0f drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices
db0036db4851 drm/bridge: ti-sn65dsi86: Rename the main driver data structure

Things not pushed:

[v5,15/20] i2c: i2c-core-of: Fix corner case of finding adapter by node
-> Can't push i2c things

[v5,14/20] drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
-> Won't work without rework. See [1]

[v5,19/20] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
-> Needs Laurent and also patch 14/20 to be resolved.

[v5,20/20] arm64: dts: qcom: Link the panel to the bridge's DDC bus
-> Needs all the rest resolved.

Let me see if I can find a way to work around the AUX channel stuff
and then I'll push a v6 of just what's left.

> Just to keep you busy and make sure you don't run out of work
> (haha) I noticed that the gpio_chip in this driver can use
> the new GPIO_REGMAP helper library with the fixes just
> landed in Torvald's tree.
>
> At your convenience and when you think there is too little
> stuff in your sn65dsi86 TODO, check out
> pinctrl-bcm63xx.c for an example of select GPIO_REGMAP
> made very simple (this works fine as long as they are bit
> offsets starting from 0).

I seem to recall you mentioning something like this. When I looked at
it in the past I wasn't convinced it would be easy. See my response
[2]. The rough summary is that I didn't think the helpers were happy
with the pm_runtime() model that I'm using. Did I get that wrong?

[1] https://lore.kernel.org/dri-devel/CAD=FV=UTmOP8LDaf-Tyx17OORQK6pJH6O_w3cP0Bu-KRYaHkYw@mail.gmail.com/
[2] https://lore.kernel.org/r/CAD=FV=VqD-dY=v23KYuTqy8aRNQJJzJ7h_UOcdEBYuK9X51MQQ@mail.gmail.com

-Doug

On Mon, May 3, 2021 at 10:41 PM Doug Anderson <dianders@chromium.org> wrote:

> > At your convenience and when you think there is too little
> > stuff in your sn65dsi86 TODO, check out
> > pinctrl-bcm63xx.c for an example of select GPIO_REGMAP
> > made very simple (this works fine as long as they are bit
> > offsets starting from 0).
>
> I seem to recall you mentioning something like this. When I looked at
> it in the past I wasn't convinced it would be easy. See my response
> [2]. The rough summary is that I didn't think the helpers were happy
> with the pm_runtime() model that I'm using. Did I get that wrong?

Yeah good point. It does seem a bit too complex for that.
Sorry for not remembering.

Yours,
Linus Walleij