| Message ID | 20230915224926.104072-1-alex.hung@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] drm/amd/display: Remove unwanted drm edid references | expand |
On Fri, 15 Sep 2023, Alex Hung <alex.hung@amd.com> wrote: > [WHY] > edid_override and drm_edid_override_connector_update, according to drm > documentation, should not be referred outside drm_edid. > > [HOW] > Remove and replace them accordingly. This can tested by IGT's > kms_hdmi_inject test. > > Signed-off-by: Alex Hung <alex.hung@amd.com> > --- > > V2 - add comments for drm_get_edid and check edid before use. > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++++++++++-------- > 1 file changed, 25 insertions(+), 19 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 5efebc06296b..b29ef87c27a9 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6444,15 +6444,24 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector) > static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) > { > struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); > + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector); > struct dc_link *dc_link = aconnector->dc_link; > struct dc_sink *dc_em_sink = aconnector->dc_em_sink; > struct edid *edid; > > - if (!connector->edid_override) > + /* > + * Note: drm_get_edid gets edid in the following order: > + * 1) override EDID if set via edid_override debugfs, > + * 2) firmware EDID if set via edid_firmware module parameter > + * 3) regular DDC read. > + */ > + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); > + if (!edid) { > + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name); > + connector->force = DRM_FORCE_OFF; Drivers aren't supposed to modify connector->force. Why would you do that anyway? This connects EDID probe success with connector forcing, and I've been repeatedly saying these are two separate things that should not be conflated. Maybe the user has set connector->force, because the display probe is flaky. This switches connector->force off if the display does not respond, undermining one of the main purposes of the whole thing. > return; > + } > > - drm_edid_override_connector_update(&aconnector->base); > - edid = aconnector->base.edid_blob_ptr->data; > aconnector->edid = edid; > > /* Update emulated (virtual) sink's EDID */ > @@ -6487,30 +6496,27 @@ static int get_modes(struct drm_connector *connector) > > static void create_eml_sink(struct amdgpu_dm_connector *aconnector) > { > + struct drm_connector *connector = &aconnector->base; > + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(&aconnector->base); > struct dc_sink_init_data init_params = { > .link = aconnector->dc_link, > .sink_signal = SIGNAL_TYPE_VIRTUAL > }; > struct edid *edid; > > - if (!aconnector->base.edid_blob_ptr) { > - /* if connector->edid_override valid, pass > - * it to edid_override to edid_blob_ptr > - */ > - > - drm_edid_override_connector_update(&aconnector->base); > - > - if (!aconnector->base.edid_blob_ptr) { > - DRM_ERROR("No EDID firmware found on connector: %s ,forcing to OFF!\n", > - aconnector->base.name); > - > - aconnector->base.force = DRM_FORCE_OFF; > - return; > - } > + /* > + * Note: drm_get_edid gets edid in the following order: > + * 1) override EDID if set via edid_override debugfs, > + * 2) firmware EDID if set via edid_firmware module parameter > + * 3) regular DDC read. > + */ > + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); > + if (!edid) { > + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name); > + connector->force = DRM_FORCE_OFF; Ditto. BR, Jani. > + return; > } > > - edid = (struct edid *) aconnector->base.edid_blob_ptr->data; > - > aconnector->edid = edid; > > aconnector->dc_em_sink = dc_link_add_remote_sink(
On 2023-09-18 04:25, Jani Nikula wrote: > On Fri, 15 Sep 2023, Alex Hung <alex.hung@amd.com> wrote: >> [WHY] >> edid_override and drm_edid_override_connector_update, according to drm >> documentation, should not be referred outside drm_edid. >> >> [HOW] >> Remove and replace them accordingly. This can tested by IGT's >> kms_hdmi_inject test. >> >> Signed-off-by: Alex Hung <alex.hung@amd.com> >> --- >> >> V2 - add comments for drm_get_edid and check edid before use. >> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++++++++++-------- >> 1 file changed, 25 insertions(+), 19 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 5efebc06296b..b29ef87c27a9 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -6444,15 +6444,24 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector) >> static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) >> { >> struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); >> + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector); >> struct dc_link *dc_link = aconnector->dc_link; >> struct dc_sink *dc_em_sink = aconnector->dc_em_sink; >> struct edid *edid; >> >> - if (!connector->edid_override) >> + /* >> + * Note: drm_get_edid gets edid in the following order: >> + * 1) override EDID if set via edid_override debugfs, >> + * 2) firmware EDID if set via edid_firmware module parameter >> + * 3) regular DDC read. >> + */ >> + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); >> + if (!edid) { >> + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name); >> + connector->force = DRM_FORCE_OFF; > > Drivers aren't supposed to modify connector->force. > > Why would you do that anyway? This connects EDID probe success with > connector forcing, and I've been repeatedly saying these are two > separate things that should not be conflated. > > Maybe the user has set connector->force, because the display probe is > flaky. This switches connector->force off if the display does not > respond, undermining one of the main purposes of the whole thing. Thanks. I will removed this and sent V3. > >> return; >> + } >> >> - drm_edid_override_connector_update(&aconnector->base); >> - edid = aconnector->base.edid_blob_ptr->data; >> aconnector->edid = edid; >> >> /* Update emulated (virtual) sink's EDID */ >> @@ -6487,30 +6496,27 @@ static int get_modes(struct drm_connector *connector) >> >> static void create_eml_sink(struct amdgpu_dm_connector *aconnector) >> { >> + struct drm_connector *connector = &aconnector->base; >> + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(&aconnector->base); >> struct dc_sink_init_data init_params = { >> .link = aconnector->dc_link, >> .sink_signal = SIGNAL_TYPE_VIRTUAL >> }; >> struct edid *edid; >> >> - if (!aconnector->base.edid_blob_ptr) { >> - /* if connector->edid_override valid, pass >> - * it to edid_override to edid_blob_ptr >> - */ >> - >> - drm_edid_override_connector_update(&aconnector->base); >> - >> - if (!aconnector->base.edid_blob_ptr) { >> - DRM_ERROR("No EDID firmware found on connector: %s ,forcing to OFF!\n", >> - aconnector->base.name); >> - >> - aconnector->base.force = DRM_FORCE_OFF; >> - return; >> - } >> + /* >> + * Note: drm_get_edid gets edid in the following order: >> + * 1) override EDID if set via edid_override debugfs, >> + * 2) firmware EDID if set via edid_firmware module parameter >> + * 3) regular DDC read. >> + */ >> + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); >> + if (!edid) { >> + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name); >> + connector->force = DRM_FORCE_OFF; > > Ditto. > > BR, > Jani. > >> + return; >> } >> >> - edid = (struct edid *) aconnector->base.edid_blob_ptr->data; >> - >> aconnector->edid = edid; >> >> aconnector->dc_em_sink = dc_link_add_remote_sink( >
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 5efebc06296b..b29ef87c27a9 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6444,15 +6444,24 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector) static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) { struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector); + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector); struct dc_link *dc_link = aconnector->dc_link; struct dc_sink *dc_em_sink = aconnector->dc_em_sink; struct edid *edid; - if (!connector->edid_override) + /* + * Note: drm_get_edid gets edid in the following order: + * 1) override EDID if set via edid_override debugfs, + * 2) firmware EDID if set via edid_firmware module parameter + * 3) regular DDC read. + */ + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); + if (!edid) { + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name); + connector->force = DRM_FORCE_OFF; return; + } - drm_edid_override_connector_update(&aconnector->base); - edid = aconnector->base.edid_blob_ptr->data; aconnector->edid = edid; /* Update emulated (virtual) sink's EDID */ @@ -6487,30 +6496,27 @@ static int get_modes(struct drm_connector *connector) static void create_eml_sink(struct amdgpu_dm_connector *aconnector) { + struct drm_connector *connector = &aconnector->base; + struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(&aconnector->base); struct dc_sink_init_data init_params = { .link = aconnector->dc_link, .sink_signal = SIGNAL_TYPE_VIRTUAL }; struct edid *edid; - if (!aconnector->base.edid_blob_ptr) { - /* if connector->edid_override valid, pass - * it to edid_override to edid_blob_ptr - */ - - drm_edid_override_connector_update(&aconnector->base); - - if (!aconnector->base.edid_blob_ptr) { - DRM_ERROR("No EDID firmware found on connector: %s ,forcing to OFF!\n", - aconnector->base.name); - - aconnector->base.force = DRM_FORCE_OFF; - return; - } + /* + * Note: drm_get_edid gets edid in the following order: + * 1) override EDID if set via edid_override debugfs, + * 2) firmware EDID if set via edid_firmware module parameter + * 3) regular DDC read. + */ + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); + if (!edid) { + DRM_ERROR("No EDID found on connector: %s, forcing to OFF!\n", connector->name); + connector->force = DRM_FORCE_OFF; + return; } - edid = (struct edid *) aconnector->base.edid_blob_ptr->data; - aconnector->edid = edid; aconnector->dc_em_sink = dc_link_add_remote_sink(
[WHY] edid_override and drm_edid_override_connector_update, according to drm documentation, should not be referred outside drm_edid. [HOW] Remove and replace them accordingly. This can tested by IGT's kms_hdmi_inject test. Signed-off-by: Alex Hung <alex.hung@amd.com> --- V2 - add comments for drm_get_edid and check edid before use. .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++++++++++-------- 1 file changed, 25 insertions(+), 19 deletions(-)