[v5,00/20] drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems

Message ID 20210423165906.2504169-1-dianders@chromium.org
Headers show
Series
  • drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems
Related show

Message

Doug Anderson April 23, 2021, 4:58 p.m.
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(-)

Comments

Sean Paul April 28, 2021, 4:32 p.m. | #1
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
Sean Paul April 28, 2021, 4:35 p.m. | #2
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
Sean Paul April 28, 2021, 4:44 p.m. | #3
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
Linus Walleij April 30, 2021, 12:58 a.m. | #4
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
Doug Anderson April 30, 2021, 1:24 a.m. | #5
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
Linus Walleij April 30, 2021, 1:27 a.m. | #6
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
Linus Walleij May 1, 2021, 12:07 p.m. | #7
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
Doug Anderson May 3, 2021, 8:41 p.m. | #8
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
Linus Walleij May 5, 2021, 12:51 p.m. | #9
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