[PATCH v3 12/12] drm/edid: split drm_add_edid_modes() to two

[Jani Nikula]

On Mon, 28 Mar 2022, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Mon, Mar 28, 2022 at 05:34:33PM +0300, Jani Nikula wrote:
>> Reduce the size of the function that actually modifies the EDID.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 42 ++++++++++++++++++++++----------------
>>  1 file changed, 24 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index d1abaa517867..d79b06f7f34c 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -5561,18 +5561,8 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>>  	return num_modes;
>>  }
>>  
>> -/**
>> - * drm_add_edid_modes - add modes from EDID data, if available
>> - * @connector: connector we're probing
>> - * @edid: EDID data
>> - *
>> - * Add the specified modes to the connector's mode list. Also fills out the
>> - * &drm_display_info structure and ELD in @connector with any information which
>> - * can be derived from the edid.
>> - *
>> - * Return: The number of modes added or 0 if we couldn't find any.
>> - */
>> -int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>> +static int drm_edid_connector_update(struct drm_connector *connector,
>> +				     const struct edid *edid)
>>  {
>>  	int num_modes = 0;
>>  	u32 quirks;
>> @@ -5581,12 +5571,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>>  		clear_eld(connector);
>>  		return 0;
>>  	}
>> -	if (!drm_edid_is_valid(edid)) {
>> -		clear_eld(connector);
>> -		drm_warn(connector->dev, "%s: EDID invalid.\n",
>> -			 connector->name);
>> -		return 0;
>> -	}
>>  
>>  	drm_edid_to_eld(connector, edid);
>>  
>> @@ -5638,6 +5622,28 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>>  
>>  	return num_modes;
>>  }
>> +
>> +/**
>> + * drm_add_edid_modes - add modes from EDID data, if available
>> + * @connector: connector we're probing
>> + * @edid: EDID data
>> + *
>> + * Add the specified modes to the connector's mode list. Also fills out the
>> + * &drm_display_info structure and ELD in @connector with any information which
>> + * can be derived from the edid.
>> + *
>> + * Return: The number of modes added or 0 if we couldn't find any.
>> + */
>> +int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>> +{
>> +	if (edid && !drm_edid_is_valid(edid)) {
>
> drm_edid_is_valid() is very poorly named since it can mutate the EDID.
> Also calling it here is kinda crazy instead of just validating when we
> originally read the EDID. But those are things for to be fixed later.

I'm trying to wrap my head around all this, but this might be the only
validation for override or firmware EDIDs. Which really should happen
much earlier. There's a lot of technical debt here.

Thanks for the review!

BR,
Jani.


>
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
>> +		drm_warn(connector->dev, "%s: EDID invalid.\n",
>> +			 connector->name);
>> +		edid = NULL;
>> +	}
>> +
>> +	return drm_edid_connector_update(connector, edid);
>> +}
>>  EXPORT_SYMBOL(drm_add_edid_modes);
>>  
>>  /**
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center