| Message ID | 20210329195255.v2.9.Ia7e9bb7cf6c51d960b9455fb0fa447cc68ece99d@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm: Fix EDID reading on ti-sn65dsi86 | expand |
On Mon, Mar 29, 2021 at 07:53:40PM -0700, Douglas Anderson wrote: > Each time we call drm_get_edid() we: > 1. Go out to the bus and ask for the EDID. > 2. Cache the EDID. > > We can improve this to actually use the cached EDID so that if > drm_get_edid() is called multiple times then we don't need to go out > to the bus over and over again. > > In normal DP/HDMI cases reading the EDID over and over again isn't > _that_ expensive so, presumably, this wasn't that critical in the > past. However for eDP going out to the bus can be expensive. This is > because eDP panels might be powered off before the EDID was requested > so we need to do power sequencing in addition to the transfer. > > In theory we should be able to cache the EDID for all types of > displays. There is already code throwing the cache away when we detect > that a display was unplugged. However, it can be noted that it's > _extra_ safe to cache the EDID for eDP since eDP isn't a hot-pluggable > interface. If we get the EDID once then we've got the EDID and we > should never need to read it again. For now we'll only use the cache > for eDP both because it's more important and extra safe. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_edid.c | 32 ++++++++++++++++++++++++++++---- > 1 file changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index c2bbe7bee7b6..fcbf468d73c9 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -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. > > - edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); > - drm_connector_update_edid_property(connector, edid); > return edid; > } > EXPORT_SYMBOL(drm_get_edid); > -- > 2.31.0.291.g576ba9dcdaf-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On Tue, Mar 30, 2021 at 7:01 AM Ville Syrjälä <ville.syrjala@linux.intel.com> 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
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index c2bbe7bee7b6..fcbf468d73c9 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -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); + } - edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); - drm_connector_update_edid_property(connector, edid); return edid; } EXPORT_SYMBOL(drm_get_edid);
Each time we call drm_get_edid() we: 1. Go out to the bus and ask for the EDID. 2. Cache the EDID. We can improve this to actually use the cached EDID so that if drm_get_edid() is called multiple times then we don't need to go out to the bus over and over again. In normal DP/HDMI cases reading the EDID over and over again isn't _that_ expensive so, presumably, this wasn't that critical in the past. However for eDP going out to the bus can be expensive. This is because eDP panels might be powered off before the EDID was requested so we need to do power sequencing in addition to the transfer. In theory we should be able to cache the EDID for all types of displays. There is already code throwing the cache away when we detect that a display was unplugged. However, it can be noted that it's _extra_ safe to cache the EDID for eDP since eDP isn't a hot-pluggable interface. If we get the EDID once then we've got the EDID and we should never need to read it again. For now we'll only use the cache for eDP both because it's more important and extra safe. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- (no changes since v1) drivers/gpu/drm/drm_edid.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)