| Message ID | 20220720155445.RFC.1.I2999ac2d08643f0c2f3fe916cca86f8c832c8142@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/2] drm/panel-edp: Allow overriding the eDP EDID | expand |
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
Hi, On Thu, Jul 21, 2022 at 4:36 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > 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. 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
On Fri, 29 Jul 2022, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Thu, Jul 21, 2022 at 4:36 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> 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. > > 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. First and foremost, please don't use connector->edid_blob_ptr for anything outside of drm_edid.c. At least not directly. It all needs to be completely hidden and abstracted away. Existing users need to be dropped. Ditto for all EDID parsing, really. The blob does actually have the data size, but most users don't look at that, and instead assume the size indicated by EDID base block extension count field matches the blob size. As to caching, I've looked at it before and I think it's best left for drivers. It's possible to cache more than just eDP, and the cache invalidation time varies. We do just that in i915. That said, I guess (purely by reading the code) we do have the same eDP EDID caching problem with edid_override in i915 as you describe here. We also have a connector init time fallback for EDID from ACPI OpRegion, and that would never get restored if thrown away. Ugh. I'm thinking of a number of alternatives for the fix, but I'd like to distill them further before sharing. BR, Jani.
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; }
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> --- drivers/gpu/drm/panel/panel-edp.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)