| Message ID | 20230905171354.2657889-1-alex.hung@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: Remove unwanted drm edid references | expand |
On Tue, 05 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. > > Signed-off-by: Alex Hung <alex.hung@amd.com> FWIW, Acked-by: Jani Nikula <jani.nikula@intel.com> and thanks for doing this! BR, Jani. > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++----------------- > 1 file changed, 2 insertions(+), 21 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 1bb1a394f55f..f6a255773242 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6372,15 +6372,12 @@ 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) > - return; > - > - drm_edid_override_connector_update(&aconnector->base); > - edid = aconnector->base.edid_blob_ptr->data; > + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); > aconnector->edid = edid; > > /* Update emulated (virtual) sink's EDID */ > @@ -6421,22 +6418,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector) > }; > 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; > - } > - } > - > edid = (struct edid *) aconnector->base.edid_blob_ptr->data; > > aconnector->edid = edid;
On 2023-09-05 13:13, Alex Hung 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. > > Signed-off-by: Alex Hung <alex.hung@amd.com> > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++----------------- > 1 file changed, 2 insertions(+), 21 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 1bb1a394f55f..f6a255773242 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6372,15 +6372,12 @@ 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) > - return; > - > - drm_edid_override_connector_update(&aconnector->base); > - edid = aconnector->base.edid_blob_ptr->data; > + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); Looks like we only call this in the case where a connector is forced, so drm_get_edid will never try to read the edid from the ddc but always gives us the override_edid. Please spell that out in the commit description so anyone else looking at the patch doesn't have to trace it themselves. > aconnector->edid = edid; > > /* Update emulated (virtual) sink's EDID */ > @@ -6421,22 +6418,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector) > }; > struct edid *edid; > > - if (!aconnector->base.edid_blob_ptr) { Can edid_blob_ptr never be NULL? Harry > - /* 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; > - } > - } > - > edid = (struct edid *) aconnector->base.edid_blob_ptr->data; > > aconnector->edid = edid;
On Fri, 15 Sep 2023, Harry Wentland <harry.wentland@amd.com> wrote: > On 2023-09-05 13:13, Alex Hung 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. >> >> Signed-off-by: Alex Hung <alex.hung@amd.com> >> --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++----------------- >> 1 file changed, 2 insertions(+), 21 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 1bb1a394f55f..f6a255773242 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -6372,15 +6372,12 @@ 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) >> - return; >> - >> - drm_edid_override_connector_update(&aconnector->base); >> - edid = aconnector->base.edid_blob_ptr->data; >> + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); > > Looks like we only call this in the case where a connector is forced, so > drm_get_edid will never try to read the edid from the ddc but always gives > us the override_edid. Please spell that out in the commit description so > anyone else looking at the patch doesn't have to trace it themselves. Connector forcing is only about forcing the connector status. The probe helper will call ->force instead of ->detect. But this has nothing to do with override_edid. That's completely orthogonal. Here, you can call drm_get_edid() if you like. Connector forcing just bypasses the DDC probe for determining connector status. If connector is forced off, you won't get the EDID, regardless of override/firmware EDID, but if it's forced on, the EDID read proceeds. And the EDID read has the priority order 1) override EDID if set via edid_override debugfs, 2) firmware EDID if set via edid_firmware module parameter, and 3) regular DDC read. BR, Jani. > >> aconnector->edid = edid; >> >> /* Update emulated (virtual) sink's EDID */ >> @@ -6421,22 +6418,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector) >> }; >> struct edid *edid; >> >> - if (!aconnector->base.edid_blob_ptr) { > > Can edid_blob_ptr never be NULL? > > Harry > >> - /* 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; >> - } >> - } >> - >> edid = (struct edid *) aconnector->base.edid_blob_ptr->data; >> >> aconnector->edid = edid; >
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 1bb1a394f55f..f6a255773242 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -6372,15 +6372,12 @@ 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) - return; - - drm_edid_override_connector_update(&aconnector->base); - edid = aconnector->base.edid_blob_ptr->data; + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc); aconnector->edid = edid; /* Update emulated (virtual) sink's EDID */ @@ -6421,22 +6418,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector) }; 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; - } - } - edid = (struct edid *) aconnector->base.edid_blob_ptr->data; aconnector->edid = edid;
[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. Signed-off-by: Alex Hung <alex.hung@amd.com> --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++----------------- 1 file changed, 2 insertions(+), 21 deletions(-)