Re: [PATCH v2 09/14] drm/edid: Use the cached EDID in drm_get_edid() if eDP

[Doug Anderson]

Hi,

On Tue, Mar 30, 2021 at 7:01 AM Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>
> > @@ -2049,15 +2049,39 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >                         struct i2c_adapter *adapter)
> >  {
> >       struct edid *edid;
> > +     size_t old_edid_size;
> > +     const struct edid *old_edid;
> >
> >       if (connector->force == DRM_FORCE_OFF)
> >               return NULL;
> >
> > -     if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
> > -             return NULL;
> > +     if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
> > +         connector->edid_blob_ptr) {
> > +             /*
> > +              * eDP devices are non-removable, or at least not something
> > +              * that's expected to be hot-pluggable. We can freely use
> > +              * the cached EDID.
> > +              *
> > +              * NOTE: technically we could probably even use the cached
> > +              * EDID even for non-eDP because the cached EDID should be
> > +              * cleared if we ever notice a display is not connected, but
> > +              * we'll use an abundance of caution and only do it for eDP.
> > +              * It's more important for eDP anyway because the EDID may not
> > +              * always be readable, like when the panel is powered down.
> > +              */
> > +             old_edid = (const struct edid *)connector->edid_blob_ptr->data;
> > +             old_edid_size = ksize(old_edid);
> > +             edid = kmalloc(old_edid_size, GFP_KERNEL);
> > +             if (edid)
> > +                     memcpy(edid, old_edid, old_edid_size);
> > +     } else {
> > +             if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
> > +                     return NULL;
> > +
> > +             edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> > +             drm_connector_update_edid_property(connector, edid);
> > +     }
>
> This is a pretty low level function. Too low level for this caching
> IMO. So I think better just do it a bit higher up like other drivers.

Fair enough. In the past I'd gotten feedback that I'd been jamming too
much stuff in my own driver instead of putting it in the core, but I'm
happy to leave the EDID caching in the driver if that's what people
prefer. It actually makes a bit of the code in the driver a bit less
awkward...

-Doug