diff mbox series

[v2,03/25] drm/edid: add struct drm_edid container

Message ID f3ecabd8a219aea678ad00f7bcdecf77b27b3c78.1652097712.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/edid: introduce struct drm_edid | expand

Commit Message

Jani Nikula May 9, 2022, 12:03 p.m. UTC
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(+)

Comments

Ankit Nautiyal May 10, 2022, 6:26 a.m. UTC | #1
LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

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 May 16, 2022, 8:11 a.m. UTC | #2
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;
diff mbox series

Patch

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;