[PATCH 2/7] drm/edid: Test for EDID header in drm_edit_probe_custom()

[Thomas Zimmermann]

Hi

Am 05.04.24 um 08:38 schrieb Jani Nikula:
> On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann at suse.de> wrote:
>> EDID read functions do not validate their return data. So they might
>> return the required number of bytes of probing, but with invalid
>> data. Therefore test for the presence of an EDID header similar to
>> the code in edid_block_check().
> I don't think the point of drm_probe_ddc() ever was to validate
> anything. It reads one byte to see if there's any response. That's all
> there is to it.
>
> All EDID validation happens in the _drm_do_get_edid() when actually
> reading the EDID.
>
> I don't think I like duplicating this behaviour in both the probe and
> the EDID read. And I'm not sure why we're giving drivers the option to
> pass a parameter whether to validate or not. Just why?

Udl doesn't use DDC, but a custom USB protocol. When queried for the 
EDID, the udl adapter sends data even if there's no monitor connected. 
All bytes are zero in this case. So the driver needs to do some sort of 
EDID validation on the received bytes to ensure that there's a monitor 
present.

drm_edid.c has all code necessary to validate the header. And there's 
the edid_fixup parameter, which I think should be respected by any 
validation helper. I'd preferably not duplicate this in udl.

Can we instead implement drm_probe_ddc() separately from 
drm_edid_probe_custom()? The former would remain as it is. The latter 
would validate unconditionally.

Best regards
Thomas

>
> BR,
> Jani.
>
>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 50 +++++++++++++++++++++++++++++---------
>>   include/drm/drm_edid.h     |  2 +-
>>   2 files changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index a3e9333f9177a..4e12d4b83a720 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1758,6 +1758,18 @@ static void edid_header_fix(void *edid)
>>   	memcpy(edid, edid_header, sizeof(edid_header));
>>   }
>>   
>> +static int edid_header_score(const u8 *header)
>> +{
>> +	int i, score = 0;
>> +
>> +	for (i = 0; i < sizeof(edid_header); i++) {
>> +		if (header[i] == edid_header[i])
>> +			score++;
>> +	}
>> +
>> +	return score;
>> +}
>> +
>>   /**
>>    * drm_edid_header_is_valid - sanity check the header of the base EDID block
>>    * @_edid: pointer to raw base EDID block
>> @@ -1769,14 +1781,8 @@ static void edid_header_fix(void *edid)
>>   int drm_edid_header_is_valid(const void *_edid)
>>   {
>>   	const struct edid *edid = _edid;
>> -	int i, score = 0;
>>   
>> -	for (i = 0; i < sizeof(edid_header); i++) {
>> -		if (edid->header[i] == edid_header[i])
>> -			score++;
>> -	}
>> -
>> -	return score;
>> +	return edid_header_score(edid->header);
>>   }
>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
>>   
>> @@ -2612,17 +2618,37 @@ EXPORT_SYMBOL(drm_edid_free);
>>    * drm_edid_probe_custom() - probe DDC presence
>>    * @read_block: EDID block read function
>>    * @context: Private data passed to the block read function
>> + * @validate: True to validate the EDID header
>>    *
>>    * Probes for EDID data. Only reads enough data to detect the presence
>> - * of the EDID channel.
>> + * of the EDID channel. Some EDID block read functions do not fail,
>> + * but return invalid data if no EDID data is available. If @validate
>> + * has been specified, drm_edid_probe_custom() validates the retrieved
>> + * EDID header before signalling success.
>>    *
>>    * Return: True on success, false on failure.
>>    */
>> -bool drm_edid_probe_custom(read_block_fn read_block, void *context)
>> +bool drm_edid_probe_custom(read_block_fn read_block, void *context, bool validate)
>>   {
>> -	unsigned char out;
>> +	u8 header[8] = {0, 0, 0, 0, 0, 0, 0, 0};
>> +	int ret;
>> +	size_t len = 1;
>> +
>> +	if (validate)
>> +		len = sizeof(header); // read full header
>> +
>> +	ret = read_block(context, header, 0, len);
>> +	if (ret)
>> +		return false;
>> +
>> +	if (validate) {
>> +		int score = edid_header_score(header);
>> +
>> +		if (score < clamp(edid_fixup, 0, 8))
>> +			return false;
>> +	}
>>   
>> -	return (read_block(context, &out, 0, 1) == 0);
>> +	return true;
>>   }
>>   EXPORT_SYMBOL(drm_edid_probe_custom);
>>   
>> @@ -2635,7 +2661,7 @@ EXPORT_SYMBOL(drm_edid_probe_custom);
>>   bool
>>   drm_probe_ddc(struct i2c_adapter *adapter)
>>   {
>> -	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
>> +	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter, false);
>>   }
>>   EXPORT_SYMBOL(drm_probe_ddc);
>>   
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 4d1797ade5f1d..299278c7ee1c2 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -412,7 +412,7 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
>>   
>>   bool
>>   drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
>> -		      void *context);
>> +		      void *context, bool validate);
>>   bool drm_probe_ddc(struct i2c_adapter *adapter);
>>   struct edid *drm_do_get_edid(struct drm_connector *connector,
>>   	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)