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 <[email protected]>
---
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
The atna33xc20 has the same problem as panel-edp where it was caching
the EDID. Let's copy the fix over. See the patch ("drm/panel-edp:
Allow overriding the eDP EDID")
NOTE: the question will of course be asked why we've got a copy of
this code. panel-samsung-atna33xc20 was purposely _copied_ from the
generic eDP panel code in response to worries that the generic code
was becoming a tangled mess (described as panel-panacea).
It should also be noted that at some point I attempted to move the
EDID caching into the core [1] but was told not to.
[1] https://lore.kernel.org/all/20210329195255.v2.9.Ia7e9bb7cf6c51d960b9455fb0fa447cc68ece99d@changeid/
Signed-off-by: Douglas Anderson <[email protected]>
---
.../gpu/drm/panel/panel-samsung-atna33xc20.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
index 5a8b978c6415..2ae62cd7c416 100644
--- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
+++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
@@ -18,6 +18,7 @@
#include <drm/display/drm_dp_helper.h>
#include <drm/drm_edid.h>
#include <drm/drm_panel.h>
+#include <drm/drm_property.h>
/* T3 VCC to HPD high is max 200 ms */
#define HPD_MAX_MS 200
@@ -36,8 +37,6 @@ struct atana33xc20_panel {
struct gpio_desc *el_on3_gpio;
struct drm_dp_aux *aux;
- struct edid *edid;
-
ktime_t powered_off_time;
ktime_t powered_on_time;
ktime_t el_on3_off_time;
@@ -241,15 +240,19 @@ static int atana33xc20_prepare(struct drm_panel *panel)
static int atana33xc20_get_modes(struct drm_panel *panel,
struct drm_connector *connector)
{
- struct atana33xc20_panel *p = to_atana33xc20(panel);
struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(panel->dev);
int num = 0;
pm_runtime_get_sync(panel->dev);
- if (!p->edid)
- p->edid = drm_get_edid(connector, &aux_ep->aux->ddc);
- num = drm_add_edid_modes(connector, p->edid);
+ /*
+ * 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, &aux_ep->aux->ddc));
+ num = drm_add_edid_modes(connector, connector->edid_blob_ptr->data);
pm_runtime_mark_last_busy(panel->dev);
pm_runtime_put_autosuspend(panel->dev);
@@ -339,8 +342,6 @@ static void atana33xc20_remove(struct dp_aux_ep_device *aux_ep)
drm_panel_remove(&panel->base);
drm_panel_disable(&panel->base);
drm_panel_unprepare(&panel->base);
-
- kfree(panel->edid);
}
static void atana33xc20_shutdown(struct dp_aux_ep_device *aux_ep)
--
2.37.0.170.g444d1eabd0-goog
On Thu, 21 Jul 2022 at 01:55, Douglas Anderson <[email protected]> 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 <[email protected]>
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
<[email protected]> wrote:
>
> On Thu, 21 Jul 2022 at 01:55, Douglas Anderson <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> wrote:
> Hi,
>
> On Thu, Jul 21, 2022 at 4:36 AM Dmitry Baryshkov
> <[email protected]> wrote:
>>
>> On Thu, 21 Jul 2022 at 01:55, Douglas Anderson <[email protected]> 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 <[email protected]>
>>
>> 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.
--
Jani Nikula, Intel Open Source Graphics Center