Re: [Intel-gfx] [PATCH v2 03/25] drm/edid: add struct drm_edid container

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

On Tue, 10 May 2022, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
> LGTM.
>
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Thanks for the review, pushed the lot already on Friday.

BR,
Jani.

>
> Regards,
>
> Ankit
>
> On 5/9/2022 5:33 PM, Jani Nikula wrote:
>> Introduce new opaque type struct drm_edid to encapsulate the EDID data
>> and the size allocated for it. The contents will be private to
>> drm_edid.c.
>>
>> There are a number of reasons for adding a container around struct edid:
>>
>> * struct edid is a raw blob pointer to data that usually originates
>>    outside of the kernel. Its size is contained within the structure.
>>
>> * There's no way to attach meta information (such as allocated memory
>>    size) to struct edid.
>>
>> * Validation of the EDID blob and its size become crucial, and it's
>>    spread all over the subsystem, with varying levels of accuracy.
>>
>> * HDMI Forum has introduced an HF-EEODB extension that defines an
>>    override EDID size within an EDID extension. The size allocated for an
>>    EDID depends on whether the allocator understands the HF-EEODB
>>    extension. Given a struct edid *, it's impossible to know how much
>>    memory was actually allocated for it.
>>
>> There are also some reasons for making the container type struct
>> drm_edid opaque and private to drm_edid.c:
>>
>> * Have only one place for creating and parsing the EDID, to avoid
>>    duplicating bugs.
>>
>> * Prepare for reading a pure DisplayID 2.0 from its own DDC address, and
>>    adding it within the same struct drm_edid container, transparently,
>>    and for all drivers.
>>
>> * With the idea that the drm_edid objects are immutable during their
>>    lifetimes, it will be possible to refcount them and reduce EDID
>>    copying everywhere (this is left for future work).
>>
>> Initially, just add the type. In follow-up, we'll start converting the
>> guts of drm_edid.c to use it, and finally add interfaces around it.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index dcef92c8887a..480fd9fbe412 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1567,6 +1567,15 @@ static const struct drm_display_mode edid_4k_modes[] = {
>>   
>>   /*** DDC fetch and block validation ***/
>>   
>> +/*
>> + * The opaque EDID type, internal to drm_edid.c.
>> + */
>> +struct drm_edid {
>> +	/* Size allocated for edid */
>> +	size_t size;
>> +	const struct edid *edid;
>> +};
>> +
>>   static int edid_extension_block_count(const struct edid *edid)
>>   {
>>   	return edid->extensions;

-- 
Jani Nikula, Intel Open Source Graphics Center