Message-ID: <CAD=FV=VUWouY58oX+6iBThz_PiDe1_zrs7Ls4gBKER8bo_Y8QQ@mail.gmail.com>
Date:   Fri, 29 Jul 2022 15:12:27 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Rob Clark <robdclark@...il.com>,
        LinusW <linus.walleij@...aro.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Sean Paul <seanpaul@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Jani Nikula <jani.nikula@...el.com>
Subject: Re: [RFC PATCH 1/2] drm/panel-edp: Allow overriding the eDP EDID

Hi,

On Thu, Jul 21, 2022 at 4:36 AM Dmitry Baryshkov
<dmitry.baryshkov@...aro.org> wrote:
>
> On Thu, 21 Jul 2022 at 01:55, Douglas Anderson <dianders@...omium.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@...omium.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.

I think the majority of drivers don't actually want the cached EDID
behavior so the edp-panel case is actually pretty rare. For everyone
else I think DRM is handling things in a pretty reasonable way.
Looking closely, it looks like there have been a bunch of patches
landed in this area recently and so I assume people are happy enough
with the current design for the majority of cases.

I guess your point though, is that the way I'm using the API right now
in ${SUBJECT} patch is a bit gross and maybe the DRM core needs a
helper of some sort for this case? Essentially what we're saying is
that we have inside knowledge this is a built-in panel and thus the
EDID will never change and it's a waste of time to read it again and
again. We could somehow tell the DRM core that.

I guess I could add a function like drm_edid_read_if_needed(). That
would essentially use the existing blob if it was there and read it
otherwise. Does that work? Basically:

def drm_edid_read_if_needed(...):
  if (connector->edid_blob_ptr)
    return dupe_edid(connector->edid_blob_ptr);
  return drm_edid_read(...);

I guess maybe we'd want a _ddc variant too.

Adding Jani since the recent patches I see touching this were his and
there are even comments there about what to do about drivers that want
to cache the EDID.


> 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.

This all looks like it's moving now, actually. Looking around at
recent changes, I see that now the property gets updated in a
different call.

Old (deprecated)
1. drm_get_edid() <-- Updates the EDID property
2. drm_add_edid_modes()

New:
1. drm_edid_read()
2. drm_edid_connector_update() <-- Updates the EDID property


 > 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.

It's certainly interesting to consider. At the moment, though, it
doesn't look super easy to do. Points to note:

1. We don't necessarily want to cache the EDID for all display types.
For builtin panels it makes sense to do so, but it's less obvious for
external displays. _In theory_ we could try to cache the EDID for
external devices if we're really certain that we'll notice when
they're unplugged / re-plugged again but I'm not convinced that all
drivers always handle this. In any case, I tend to assume that when
we're dealing with external displays we're a little less interested in
trying to optimize all of the milliseconds away. If nothing else there
are hundreds of milliseconds of hotplug detect debounce happening for
external displays. Yes, we could have a rule about only caching the
EDID only for eDP displays but then the motivation of moving it out of
edp-panel and to drm_bridge_connector is a lot less.

2. At the moment, drm_bridge_connector only calls get_modes() if it
doesn't have get_edid() implemented. At the moment the panel-edp code
actually _combines_ the EDID and any hardcoded modes that were
specified. I think we'd have to resolve this difference if we do what
you suggest. The panel-edp behavior comes from before the split out of
panel-simple and dates from 2013 when panel-simple was first added.
Certainly we could arbitrarily change one behavior or the other but I
don't know what the fallout would be.

3. We still don't have universal conversion to panel_bridge and
drm_bridge_connector. Some drivers are still calling the panel
functions directly. Until everything is converted it will be extra
cruft / scaffolding to make this change without breaking those calling
the panel directly. I don't think there's enough of a hurry to do this
that it's worth the extra cruft. There just aren't that many built-in
panels that read an EDID that aren't handled by the generic panel-edp.


-Doug

Powered by blists - more mailing lists