Re: [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
On 03/28, Jani Nikula wrote:
> On Wed, 27 Mar 2024, Melissa Wen wrote:
> > Replace raw edid handling (struct edid) with the opaque EDID type
> > (struct drm_edid) on amdgpu_dm_connector for consistency. It may also
> > prevent mismatch of approaches in different parts of the driver code.
> > Working in progress. It was only exercised with IGT tests.
> >
> > v2: use const to fix warnings (Alex Hung)
> > v3: fix general protection fault on mst
> >
> > Signed-off-by: Melissa Wen
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +--
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +-
> > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 8 +-
> > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 ++---
> > 4 files changed, 67 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 280562707cd0..bbbf9c9d40d5 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect(
> > aconnector->dc_sink = sink;
> > dc_sink_retain(aconnector->dc_sink);
> > if (sink->dc_edid.length == 0) {
> > - aconnector->edid = NULL;
> > + drm_edid_free(aconnector->edid);
>
> If aconnector->edid is used as some kind of cache, it might be prudent
> to still set it to NULL.
Makes sense. Thanks for pointing it out.
>
> It's up to the amd maintainers, but personally I've renamed any ->edid
> fields to ->drm_edid when I've done these conversions to ensure every
> single place is covered, and also later stable backports will give a
> build failure if the change is not taken into account.
I see. Good points. I'll follow your advice and do this name change in
the next version.
>
> > if (aconnector->dc_link->aux_mode) {
> > drm_dp_cec_unset_edid(
> > >dm_dp_aux.aux);
> > }
> > } else {
> > - aconnector->edid =
> > - (struct edid *)sink->dc_edid.raw_edid;
> > + const struct edid *edid = (const struct edid
> > *)sink->dc_edid.raw_edid;
> > + aconnector->edid = drm_edid_alloc(edid,
> > sink->dc_edid.length);
> >
> > + /* FIXME: Get rid of drm_edid_raw() */
> > if (aconnector->dc_link->aux_mode)
> > drm_dp_cec_set_edid(>dm_dp_aux.aux,
> > - aconnector->edid);
> > +
> > drm_edid_raw(aconnector->edid));
>
> The goal should be to switch to use CEC functions that take the source
> physical address directly instead of parsing the EDID.
Yes, I understand I have the same goal as in the next patch:
- https://lore.kernel.org/dri-devel/20240327214953.367126-5-m...@igalia.com/
Am I missing something?
>
> > }
> >
> > if (!aconnector->timing_requested) {
> > @@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect(
> > "failed to create
> > aconnector->requested_timing\n");
> > }
> >
> > - drm_connector_update_edid_property(connector, aconnector->edid);
> > + drm_edid_connector_update(connector, aconnector->edid);
> > amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
> > update_connector_ext_caps(aconnector);
> > } else {
> > drm_dp_cec_unset_edid(>dm_dp_aux.aux);
> > amdgpu_dm_update_freesync_caps(connector, NULL);
> > - drm_connector_update_edid_property(connector, NULL);
> > + drm_edid_connector_update(connector, NULL);
> > aconnector->num_modes = 0;
> > dc_sink_release(aconnector->dc_sink);
> > aconnector->dc_sink = NULL;
> > - aconnector->edid = NULL;
> > + drm_edid_free(aconnector->edid);
> > kfree(aconnector->timing_requested);
> > aconnector->timing_requested = NULL;
> > /* Set CP to DESIRED if it was ENABLED, so we can re-enable it
> > again on hotplug */
> > @@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct
> > drm_connector *connector)
> > struct amdgpu_dm_connector *aconnector =
> > to_amdgpu_dm_connector(connector);
> > struct dc_link *dc_link = aconnector->dc_link;
> > struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
> > - struct edid *edid;
> > - struct i2c_adapter *ddc;
> > -
> > - if (dc_link && dc_link->aux_mode)
> > - ddc = >dm_dp_aux.aux.ddc;
> > - else
> > - ddc = >i2c->base;
> > + const struct drm_edid *drm_edid;
> >
> > /*
> > * Note: drm_get_edid gets edid in
Re: [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
On Wed, 27 Mar 2024, Melissa Wen wrote:
> Replace raw edid handling (struct edid) with the opaque EDID type
> (struct drm_edid) on amdgpu_dm_connector for consistency. It may also
> prevent mismatch of approaches in different parts of the driver code.
> Working in progress. It was only exercised with IGT tests.
>
> v2: use const to fix warnings (Alex Hung)
> v3: fix general protection fault on mst
>
> Signed-off-by: Melissa Wen
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +--
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +-
> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 8 +-
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 ++---
> 4 files changed, 67 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 280562707cd0..bbbf9c9d40d5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect(
> aconnector->dc_sink = sink;
> dc_sink_retain(aconnector->dc_sink);
> if (sink->dc_edid.length == 0) {
> - aconnector->edid = NULL;
> + drm_edid_free(aconnector->edid);
If aconnector->edid is used as some kind of cache, it might be prudent
to still set it to NULL.
It's up to the amd maintainers, but personally I've renamed any ->edid
fields to ->drm_edid when I've done these conversions to ensure every
single place is covered, and also later stable backports will give a
build failure if the change is not taken into account.
> if (aconnector->dc_link->aux_mode) {
> drm_dp_cec_unset_edid(
> >dm_dp_aux.aux);
> }
> } else {
> - aconnector->edid =
> - (struct edid *)sink->dc_edid.raw_edid;
> + const struct edid *edid = (const struct edid
> *)sink->dc_edid.raw_edid;
> + aconnector->edid = drm_edid_alloc(edid,
> sink->dc_edid.length);
>
> + /* FIXME: Get rid of drm_edid_raw() */
> if (aconnector->dc_link->aux_mode)
> drm_dp_cec_set_edid(>dm_dp_aux.aux,
> - aconnector->edid);
> +
> drm_edid_raw(aconnector->edid));
The goal should be to switch to use CEC functions that take the source
physical address directly instead of parsing the EDID.
> }
>
> if (!aconnector->timing_requested) {
> @@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect(
> "failed to create
> aconnector->requested_timing\n");
> }
>
> - drm_connector_update_edid_property(connector, aconnector->edid);
> + drm_edid_connector_update(connector, aconnector->edid);
> amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
> update_connector_ext_caps(aconnector);
> } else {
> drm_dp_cec_unset_edid(>dm_dp_aux.aux);
> amdgpu_dm_update_freesync_caps(connector, NULL);
> - drm_connector_update_edid_property(connector, NULL);
> + drm_edid_connector_update(connector, NULL);
> aconnector->num_modes = 0;
> dc_sink_release(aconnector->dc_sink);
> aconnector->dc_sink = NULL;
> - aconnector->edid = NULL;
> + drm_edid_free(aconnector->edid);
> kfree(aconnector->timing_requested);
> aconnector->timing_requested = NULL;
> /* Set CP to DESIRED if it was ENABLED, so we can re-enable it
> again on hotplug */
> @@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct
> drm_connector *connector)
> struct amdgpu_dm_connector *aconnector =
> to_amdgpu_dm_connector(connector);
> struct dc_link *dc_link = aconnector->dc_link;
> struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
> - struct edid *edid;
> - struct i2c_adapter *ddc;
> -
> - if (dc_link && dc_link->aux_mode)
> - ddc = >dm_dp_aux.aux.ddc;
> - else
> - ddc = >i2c->base;
> + const struct drm_edid *drm_edid;
>
> /*
>* Note: drm_get_edid gets edid in the following order:
> @@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct
> drm_connector *connector)
>* 2) firmware EDID if set via edid_firmware module parameter
>* 3) regular DDC read.
>*/
> - edid = drm_get_edid(connector, ddc);
> - if (!edid) {
> + drm_edid = drm_edid_read(connector);
You should call drm_edid_connector_update() here to clear the EDID
[RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
Replace raw edid handling (struct edid) with the opaque EDID type
(struct drm_edid) on amdgpu_dm_connector for consistency. It may also
prevent mismatch of approaches in different parts of the driver code.
Working in progress. It was only exercised with IGT tests.
v2: use const to fix warnings (Alex Hung)
v3: fix general protection fault on mst
Signed-off-by: Melissa Wen
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +--
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +-
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 8 +-
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 ++---
4 files changed, 67 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 280562707cd0..bbbf9c9d40d5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect(
aconnector->dc_sink = sink;
dc_sink_retain(aconnector->dc_sink);
if (sink->dc_edid.length == 0) {
- aconnector->edid = NULL;
+ drm_edid_free(aconnector->edid);
if (aconnector->dc_link->aux_mode) {
drm_dp_cec_unset_edid(
>dm_dp_aux.aux);
}
} else {
- aconnector->edid =
- (struct edid *)sink->dc_edid.raw_edid;
+ const struct edid *edid = (const struct edid
*)sink->dc_edid.raw_edid;
+ aconnector->edid = drm_edid_alloc(edid,
sink->dc_edid.length);
+ /* FIXME: Get rid of drm_edid_raw() */
if (aconnector->dc_link->aux_mode)
drm_dp_cec_set_edid(>dm_dp_aux.aux,
- aconnector->edid);
+
drm_edid_raw(aconnector->edid));
}
if (!aconnector->timing_requested) {
@@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect(
"failed to create
aconnector->requested_timing\n");
}
- drm_connector_update_edid_property(connector, aconnector->edid);
+ drm_edid_connector_update(connector, aconnector->edid);
amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
update_connector_ext_caps(aconnector);
} else {
drm_dp_cec_unset_edid(>dm_dp_aux.aux);
amdgpu_dm_update_freesync_caps(connector, NULL);
- drm_connector_update_edid_property(connector, NULL);
+ drm_edid_connector_update(connector, NULL);
aconnector->num_modes = 0;
dc_sink_release(aconnector->dc_sink);
aconnector->dc_sink = NULL;
- aconnector->edid = NULL;
+ drm_edid_free(aconnector->edid);
kfree(aconnector->timing_requested);
aconnector->timing_requested = NULL;
/* Set CP to DESIRED if it was ENABLED, so we can re-enable it
again on hotplug */
@@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct
drm_connector *connector)
struct amdgpu_dm_connector *aconnector =
to_amdgpu_dm_connector(connector);
struct dc_link *dc_link = aconnector->dc_link;
struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
- struct edid *edid;
- struct i2c_adapter *ddc;
-
- if (dc_link && dc_link->aux_mode)
- ddc = >dm_dp_aux.aux.ddc;
- else
- ddc = >i2c->base;
+ const struct drm_edid *drm_edid;
/*
* Note: drm_get_edid gets edid in the following order:
@@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct
drm_connector *connector)
* 2) firmware EDID if set via edid_firmware module parameter
* 3) regular DDC read.
*/
- edid = drm_get_edid(connector, ddc);
- if (!edid) {
+ drm_edid = drm_edid_read(connector);
+
+ if (!drm_edid) {
DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
return;
}
-
- aconnector->edid = edid;
-
+ aconnector->edid = drm_edid;
+ drm_edid_connector_update(connector, drm_edid);
/* Update emulated (virtual) sink's EDID */
if (dc_em_sink && dc_link) {
+ const struct edid *edid = drm_edid_raw(drm_edid); // FIXME: Get
rid of drm_edid_raw()
+
memset(_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps));
- memmove(dc_em_sink->dc_edid.raw_edid, edid, (edid->extensions +
1) * EDID_LENGTH);
+
