[PATCH][V2] drm/amd/display: Remove unwanted drm edid references

[Alex Hung]

On 2023-09-18 04:25, Jani Nikula wrote:
> On Fri, 15 Sep 2023, Alex Hung <alex.hung at 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 at 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(
>