Re: [PATCH 05/11] drm/hisilicon/hibmc: convert to struct drm_edid

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 14 May 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 14.05.24 um 14:55 schrieb Jani Nikula:
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>
>> ---
>>
>> Cc: Xinliang Liu <xinliang.liu@linaro.org>
>> Cc: Tian Tao <tiantao6@hisilicon.com>
>> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: Yongqin Liu <yongqin.liu@linaro.org>
>> Cc: John Stultz <jstultz@google.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c    | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> index 94e2c573a7af..409c551c92af 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
>> @@ -24,14 +24,16 @@
>>   
>>   static int hibmc_connector_get_modes(struct drm_connector *connector)
>
> Could this function be replaced with a call to 
> drm_connector_helper_get_modes()? With the patch, the only difference is 
> the call to the _noedid() function, which the DRM core does 
> automatically. There will still be a difference in the maximum 
> resolution, but standardizing on 1024x768 seems preferable to me.

Looks like it might work, but personally, I'm trying to shy away from
any further cleanups than just switching to struct drm_edid. I've got my
plate pretty full already.

BR,
Jani.


>
> Best regards
> Thomas
>
>>   {
>> -	int count;
>> -	void *edid;
>>   	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
>> +	const struct drm_edid *drm_edid;
>> +	int count;
>> +
>> +	drm_edid = drm_edid_read_ddc(connector, &hibmc_connector->adapter);
>>   
>> -	edid = drm_get_edid(connector, &hibmc_connector->adapter);
>> -	if (edid) {
>> -		drm_connector_update_edid_property(connector, edid);
>> -		count = drm_add_edid_modes(connector, edid);
>> +	drm_edid_connector_update(connector, drm_edid);
>> +
>> +	if (drm_edid) {
>> +		count = drm_edid_connector_add_modes(connector);
>>   		if (count)
>>   			goto out;
>>   	}
>> @@ -42,7 +44,8 @@ static int hibmc_connector_get_modes(struct drm_connector *connector)
>>   	drm_set_preferred_mode(connector, 1024, 768);
>>   
>>   out:
>> -	kfree(edid);
>> +	drm_edid_free(drm_edid);
>> +
>>   	return count;
>>   }
>>   

-- 
Jani Nikula, Intel