Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220720155445.RFC.1.I2999ac2d08643f0c2f3fe916cca86f8c832c8142@changeid>
In-Reply-To: <20220720155445.RFC.1.I2999ac2d08643f0c2f3fe916cca86f8c832c8142@changeid>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Thu, 21 Jul 2022 14:36:32 +0300
Message-ID: <CAA8EJpombZYHKXKd=rLte0pUaXjep0t7+H-uz_sFTuJwjH3y9A@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] drm/panel-edp: Allow overriding the eDP EDID
To:     Douglas Anderson <dianders@chromium.org>
Cc:     dri-devel@lists.freedesktop.org, ville.syrjala@linux.intel.com,
        sam@ravnborg.org, robdclark@gmail.com, linus.walleij@linaro.org,
        thierry.reding@gmail.com, bjorn.andersson@linaro.org,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>,
        Sean Paul <seanpaul@chromium.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 21 Jul 2022 at 01:55, Douglas Anderson <dianders@chromium.org> wrote:
>
> I found that writing to `/sys/kernel/debug/dri/*/eDP*/edid_override`
> wasn't working for me. I could see the new EDID take effect in
> `/sys/class/drm/card*-eDP*/edid` but userspace wasn't seeing it..
>
> The problem was that panel-edp was caching the EDID that it read and
> using that over and over again.
>
> Let's change panel-edp to look at the EDID that's stored in the
> connector. This is still a cache, which is important since this
> function is called multiple times and readin the EDID is slow, but
> this property is automatically updated by drm_get_edid() (which reads
> the EDID) and also updated when writing the edid_override in debugfs.
>
> Fixes: 63358e24ee79 ("drm/panel: panel-simple: Cache the EDID as long as we retain power")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

A different proposal for you to consider:
Change drm_get_edid/drm_do_get_edid to return int rather than struct
edid, while caching the EDID in the connector. Or maybe add a new API
drm_read_edid() and make drm_get_edid() deprecated in favour of it.
The goal should be to let all drivers use connector-cached EDID rather
than getting  the EDID, parsing it and kfree()ing it immediately
afterwards.
Most probably we should be able to move
drm_connector_update_edid_property() into drm_do_get_edid() and drop
it from the rest of the code. This might require additional thought
about locking, to ensure that nobody pulls the cached edid out from
under our feet.

Extra "bonus" points to consider:
- Maybe it's time to add get_edid() to the drm_panel interface, teach
panel_bridge about it and let drm_bridge_connector handle all the
details?

So, while this looks like a longer path, I think it's worth checking
that we can refactor this piece of code.

> ---
>
>  drivers/gpu/drm/panel/panel-edp.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 3626469c4cc2..12241c1e32f7 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -226,8 +226,6 @@ struct panel_edp {
>
>         const struct edp_panel_entry *detected_panel;
>
> -       struct edid *edid;
> -
>         struct drm_display_mode override_mode;
>
>         enum drm_panel_orientation orientation;
> @@ -580,11 +578,19 @@ static int panel_edp_get_modes(struct drm_panel *panel,
>         if (p->ddc) {
>                 pm_runtime_get_sync(panel->dev);
>
> -               if (!p->edid)
> -                       p->edid = drm_get_edid(connector, p->ddc);
> +               if (!connector->edid_blob_ptr) {
> +                       /*
> +                        * We read the EDID and then immediately free it,
> +                        * relying on the side effect of drm_get_edid() to store
> +                        * a copy in connector->edid_blob_ptr. We always use
> +                        * the copy cached in the connector since that allows
> +                        * the edid_override to work.
> +                        */
> +                       kfree(drm_get_edid(connector, p->ddc));
> +               }
>
> -               if (p->edid)
> -                       num += drm_add_edid_modes(connector, p->edid);
> +               if (connector->edid_blob_ptr)
> +                       num += drm_add_edid_modes(connector, connector->edid_blob_ptr->data);
>
>                 pm_runtime_mark_last_busy(panel->dev);
>                 pm_runtime_put_autosuspend(panel->dev);
> @@ -926,9 +932,6 @@ static int panel_edp_remove(struct device *dev)
>         if (panel->ddc && (!panel->aux || panel->ddc != &panel->aux->ddc))
>                 put_device(&panel->ddc->dev);
>
> -       kfree(panel->edid);
> -       panel->edid = NULL;
> -
>         return 0;
>  }
>
> --
> 2.37.0.170.g444d1eabd0-goog
>


--
With best wishes
Dmitry