| Message ID | 20210423095743.v5.1.I9e6af2529d6c61e5daf86a15a1211121c5223b9a@changeid |
|---|---|
| State | Accepted |
| Commit | 70e12560126685e982ee680e0e40049edbd66cf0 |
| Headers | show |
| Series | drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems | expand |
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
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
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);
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(-)