| Message ID | 20210423165906.2504169-1-dianders@chromium.org |
|---|---|
| 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 1:00 PM Douglas Anderson <dianders@chromium.org> wrote: > > When I added support for the hpd-gpio to simple-panel in commit > 48834e6084f1 ("drm/panel-simple: Support hpd-gpios for delaying > prepare()"), I added a special case to handle a circular dependency I > was running into on the ti-sn65dsi86 bridge chip. On my board the > hpd-gpio is actually provided by the bridge chip. That was causing > some circular dependency problems that I had to work around by getting > the hpd-gpio late. > > I've now reorganized the ti-sn65dsi86 bridge chip driver to be a > collection of sub-drivers. Now the GPIO part can probe separately and > that breaks the chain. Let's get rid of the old code to clean things > up. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Reviewed-by: Sean Paul <seanpaul@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/panel/panel-simple.c | 24 +++++------------------- > 1 file changed, 5 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 9746eda6f675..bd208abcbf07 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -366,8 +366,7 @@ static int panel_simple_unprepare(struct drm_panel *panel) > return 0; > } > > -static int panel_simple_get_hpd_gpio(struct device *dev, > - struct panel_simple *p, bool from_probe) > +static int panel_simple_get_hpd_gpio(struct device *dev, struct panel_simple *p) > { > int err; > > @@ -375,17 +374,10 @@ static int panel_simple_get_hpd_gpio(struct device *dev, > if (IS_ERR(p->hpd_gpio)) { > err = PTR_ERR(p->hpd_gpio); > > - /* > - * If we're called from probe we won't consider '-EPROBE_DEFER' > - * to be an error--we'll leave the error code in "hpd_gpio". > - * When we try to use it we'll try again. This allows for > - * circular dependencies where the component providing the > - * hpd gpio needs the panel to init before probing. > - */ > - if (err != -EPROBE_DEFER || !from_probe) { > + if (err != -EPROBE_DEFER) > dev_err(dev, "failed to get 'hpd' GPIO: %d\n", err); > - return err; > - } > + > + return err; > } > > return 0; > @@ -416,12 +408,6 @@ static int panel_simple_prepare_once(struct panel_simple *p) > msleep(delay); > > if (p->hpd_gpio) { > - if (IS_ERR(p->hpd_gpio)) { > - err = panel_simple_get_hpd_gpio(dev, p, false); > - if (err) > - goto error; > - } > - > if (p->desc->delay.hpd_absent_delay) > hpd_wait_us = p->desc->delay.hpd_absent_delay * 1000UL; > else > @@ -682,7 +668,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) > > panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd"); > if (!panel->no_hpd) { > - err = panel_simple_get_hpd_gpio(dev, panel, true); > + err = panel_simple_get_hpd_gpio(dev, panel); > if (err) > return err; > } > -- > 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 1:00 PM Douglas Anderson <dianders@chromium.org> wrote: > > I don't believe that it ever makes sense to read the EDID when a panel > is not powered and the powering on of the panel is the job of > prepare(). Let's make sure that this happens before we try to read the > EDID. We use the pm_runtime functions directly rather than directly > calling the normal prepare() function because the pm_runtime functions > are definitely refcounted whereas it's less clear if the prepare() one > is. > > NOTE: I'm not 100% sure how EDID reading was working for folks in the > past, but I can only assume that it was failing on the initial attempt > and then working only later. This patch, presumably, will fix that. If > some panel out there really can read the EDID without powering up and > it's a big advantage to preserve the old behavior we can add a > per-panel flag. It appears that providing the DDC bus to the panel in > the past was somewhat uncommon in any case. > Maybe some combination of drivers caching the EDID for panels while they're already powered and overly broad pm runtime references? At any rate, this makes sense to me, Reviewed-by: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > (no changes since v1) > > drivers/gpu/drm/panel/panel-simple.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 4de33c929a59..a12dfe8b8d90 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -510,12 +510,18 @@ static int panel_simple_get_modes(struct drm_panel *panel, > > /* probe EDID if a DDC bus is available */ > if (p->ddc) { > - struct edid *edid = drm_get_edid(connector, 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); > } > + > + pm_runtime_mark_last_busy(panel->dev); > + pm_runtime_put_autosuspend(panel->dev); > } > > /* add hard-coded panel modes */ > -- > 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 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
The primary goal of this series is to try to properly fix EDID reading for eDP panels using the ti-sn65dsi86 bridge. Previously we had a patch that added EDID reading but it turned out not to work at bootup. This caused some extra churn at bootup as we tried (and failed) to read the EDID several times and also ended up forcing us to use the hardcoded mode at boot. With this patch series I believe EDID reading is reliable at boot now and we never use the hardcoded mode. This series is the logical successor to the 3-part series containing the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk") [1] though only one actual patch is the same between the two. This series starts out with some general / obvious fixes and moves on to some more specific and maybe controversial ones. I wouldn't object to some of the earlier ones landing if they look ready. This patch was developed agains linuxnext (next-20210416) with drm-misc-next (as of 20210423) merged in on a sc7180-trogdor-lazor device. To get things booting for me, I had to use Stephen's patch [2] to keep from crashing but otherwise all the patches I needed were here. Primary change between v2 and v3 is to stop doing the EDID caching in the core. I also added Andrzej's review tags. Between v3 and v4 this series grew a whole lot. I changed it so that the EDID reading is actually driven by the panel driver now as was suggested by Andrzej. While I still believe that the old approach wasn't too bad I'm still switching. Why? The main reason is that I think it's useful in general for the panel code to have access to the DDC bus and to be able to read the EDID. This may allow us to more easily have the panel code support multiple sources of panels--it can read the EDID and possibly adjust timings based on the model ID. It also allows the panel code (or perhaps backlight code?) to send DDC commands if they are need for a particular panel. At the moment, once the panel is provided the DDC bus then existing code will assume that it should be in charge of reading the EDID. While it doesn't have to work that way, it seems sane to build on what's already there. In order to expose the DDC bus to the panel, I had to solve a bunch of chicken-and-egg problems in terms of probe ordering between the bridge and the panel. I've broken the bridge driver into several sub drivers to make this happen. At the moment the sub-drivers are just there to solve the probe problem, but conceivably someone could use them to break the driver up in the future if need be. Between v4 and v5, high-level view of changes. - Some of the early patches landed, so dropped from series. - New pm_runtime_disable() fix (fixed a patch that already landed). - Added Bjorn's tags to most patches - Fixed problems when building as a module. - Reordered debugfs patch and fixed error handling there. - Dropped last patch. I'm not convinced it's safe w/out more work. I apologize in advance for the length of this series. Hopefully you can see that there are only so many because they're broken up into nice and reviewable bite-sized-chunks. :-) *** NOTE ***: my hope is to actually land patches that have been reviewed sometime mid-to-late next week. Please shout if you think this is too much of a rush and you know of someone who is planning to review that needs more time. [1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/ [2] https://lore.kernel.org/r/161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com/ Changes in v5: - Missing pm_runtime_disable() patch new for v5. - Reordered to debugfs change to avoid transient issue - Don't print debugfs creation errors. - Handle NULL from debugfs_create_dir() which is documented possible. - Rebased atop the pm_runtime patch, which got reordered. - Fix module compile problems (Bjorn + kbuild bot) - Remove useless MODULE_DEVICE_TABLE (Bjorn). - Fix module compile problems (Bjorn + kbuild bot) - Remove useless MODULE_DEVICE_TABLE (Bjorn). Douglas Anderson (20): drm/panel: panel-simple: Add missing pm_runtime_disable() calls drm/bridge: ti-sn65dsi86: Rename the main driver data structure drm/bridge: ti-sn65dsi86: More renames in prep for sub-devices drm/bridge: ti-sn65dsi86: Use devm to do our runtime_disable drm/bridge: ti-sn65dsi86: Clean debugfs code drm/bridge: ti-sn65dsi86: Add local var for "dev" to simplify probe drm/bridge: ti-sn65dsi86: Cleanup managing of drvdata drm/bridge: ti-sn65dsi86: Move all the chip-related init to the start drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers drm/panel: panel-simple: Get rid of hacky HPD chicken-and-egg code drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend drm/bridge: ti-sn65dsi86: Code motion of refclk management functions drm/bridge: ti-sn65dsi86: If refclk, DP AUX can happen w/out pre-enable drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev i2c: i2c-core-of: Fix corner case of finding adapter by node drm/panel: panel-simple: Remove extra call: drm_connector_update_edid_property() drm/panel: panel-simple: Power the panel when reading the EDID drm/panel: panel-simple: Cache the EDID as long as we retain power drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC arm64: dts: qcom: Link the panel to the bridge's DDC bus arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 + drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 767 ++++++++++++------- drivers/gpu/drm/panel/panel-simple.c | 49 +- drivers/i2c/i2c-core-of.c | 17 +- 5 files changed, 538 insertions(+), 297 deletions(-)