diff mbox series

[REBASE,7/7] drm/edid: make drm_edid_are_equal() more convenient for its single user

Message ID 1011a285d30babce3aabd8218abb7ece7dcf58a2.1713259151.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/edid: cleanups, rebase | expand

Commit Message

Jani Nikula April 16, 2024, 9:20 a.m. UTC
Repurpose drm_edid_are_equal() to be more helpful for its single user,
and rename drm_edid_eq(). Functionally deduce the length from the blob
size, not the blob data, making it more robust against any errors.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

Comments

Thomas Zimmermann April 16, 2024, 12:21 p.m. UTC | #1
Hi

Am 16.04.24 um 11:20 schrieb Jani Nikula:
> Repurpose drm_edid_are_equal() to be more helpful for its single user,
> and rename drm_edid_eq(). Functionally deduce the length from the blob
> size, not the blob data, making it more robust against any errors.

Could be squashed into patch 6.

Best regards
Thomas

>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------
>   1 file changed, 15 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 463fbad85d90..513590931cc5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1820,30 +1820,20 @@ static bool edid_block_is_zero(const void *edid)
>   	return !memchr_inv(edid, 0, EDID_LENGTH);
>   }
>   
> -/**
> - * drm_edid_are_equal - compare two edid blobs.
> - * @edid1: pointer to first blob
> - * @edid2: pointer to second blob
> - * This helper can be used during probing to determine if
> - * edid had changed.
> - */
> -static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
> +static bool drm_edid_eq(const struct drm_edid *drm_edid,
> +			const void *raw_edid, size_t raw_edid_size)
>   {
> -	int edid1_len, edid2_len;
> -	bool edid1_present = edid1 != NULL;
> -	bool edid2_present = edid2 != NULL;
> +	bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
> +	bool edid2_present = raw_edid && raw_edid_size;
>   
>   	if (edid1_present != edid2_present)
>   		return false;
>   
> -	if (edid1) {
> -		edid1_len = edid_size(edid1);
> -		edid2_len = edid_size(edid2);
> -
> -		if (edid1_len != edid2_len)
> +	if (edid1_present) {
> +		if (drm_edid->size != raw_edid_size)
>   			return false;
>   
> -		if (memcmp(edid1, edid2, edid1_len))
> +		if (memcmp(drm_edid->edid, raw_edid, drm_edid->size))
>   			return false;
>   	}
>   
> @@ -6936,15 +6926,14 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
>   	int ret;
>   
>   	if (connector->edid_blob_ptr) {
> -		const struct edid *old_edid = connector->edid_blob_ptr->data;
> -
> -		if (old_edid) {
> -			if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) {
> -				connector->epoch_counter++;
> -				drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
> -					    connector->base.id, connector->name,
> -					    connector->epoch_counter);
> -			}
> +		const void *old_edid = connector->edid_blob_ptr->data;
> +		size_t old_edid_size = connector->edid_blob_ptr->length;
> +
> +		if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) {
> +			connector->epoch_counter++;
> +			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
> +				    connector->base.id, connector->name,
> +				    connector->epoch_counter);
>   		}
>   	}
>
Jani Nikula April 16, 2024, 12:27 p.m. UTC | #2
On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 16.04.24 um 11:20 schrieb Jani Nikula:
>> Repurpose drm_edid_are_equal() to be more helpful for its single user,
>> and rename drm_edid_eq(). Functionally deduce the length from the blob
>> size, not the blob data, making it more robust against any errors.
>
> Could be squashed into patch 6.

Ack.

Thanks for the review. I'll hold of on resending these until there are
some R-b's... I've send them a few times already with no comments. :(

BR,
Jani.

>
> Best regards
> Thomas
>
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------
>>   1 file changed, 15 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 463fbad85d90..513590931cc5 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1820,30 +1820,20 @@ static bool edid_block_is_zero(const void *edid)
>>   	return !memchr_inv(edid, 0, EDID_LENGTH);
>>   }
>>   
>> -/**
>> - * drm_edid_are_equal - compare two edid blobs.
>> - * @edid1: pointer to first blob
>> - * @edid2: pointer to second blob
>> - * This helper can be used during probing to determine if
>> - * edid had changed.
>> - */
>> -static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
>> +static bool drm_edid_eq(const struct drm_edid *drm_edid,
>> +			const void *raw_edid, size_t raw_edid_size)
>>   {
>> -	int edid1_len, edid2_len;
>> -	bool edid1_present = edid1 != NULL;
>> -	bool edid2_present = edid2 != NULL;
>> +	bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
>> +	bool edid2_present = raw_edid && raw_edid_size;
>>   
>>   	if (edid1_present != edid2_present)
>>   		return false;
>>   
>> -	if (edid1) {
>> -		edid1_len = edid_size(edid1);
>> -		edid2_len = edid_size(edid2);
>> -
>> -		if (edid1_len != edid2_len)
>> +	if (edid1_present) {
>> +		if (drm_edid->size != raw_edid_size)
>>   			return false;
>>   
>> -		if (memcmp(edid1, edid2, edid1_len))
>> +		if (memcmp(drm_edid->edid, raw_edid, drm_edid->size))
>>   			return false;
>>   	}
>>   
>> @@ -6936,15 +6926,14 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
>>   	int ret;
>>   
>>   	if (connector->edid_blob_ptr) {
>> -		const struct edid *old_edid = connector->edid_blob_ptr->data;
>> -
>> -		if (old_edid) {
>> -			if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) {
>> -				connector->epoch_counter++;
>> -				drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
>> -					    connector->base.id, connector->name,
>> -					    connector->epoch_counter);
>> -			}
>> +		const void *old_edid = connector->edid_blob_ptr->data;
>> +		size_t old_edid_size = connector->edid_blob_ptr->length;
>> +
>> +		if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) {
>> +			connector->epoch_counter++;
>> +			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
>> +				    connector->base.id, connector->name,
>> +				    connector->epoch_counter);
>>   		}
>>   	}
>>
Thomas Zimmermann April 16, 2024, 12:47 p.m. UTC | #3
Hi

Am 16.04.24 um 14:27 schrieb Jani Nikula:
> On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 16.04.24 um 11:20 schrieb Jani Nikula:
>>> Repurpose drm_edid_are_equal() to be more helpful for its single user,
>>> and rename drm_edid_eq(). Functionally deduce the length from the blob
>>> size, not the blob data, making it more robust against any errors.
>> Could be squashed into patch 6.
> Ack.
>
> Thanks for the review. I'll hold of on resending these until there are
> some R-b's... I've send them a few times already with no comments. :(

Feel free to add

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

to the series.

Best regards
Thomas

>
> BR,
> Jani.
>
>> Best regards
>> Thomas
>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------
>>>    1 file changed, 15 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 463fbad85d90..513590931cc5 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -1820,30 +1820,20 @@ static bool edid_block_is_zero(const void *edid)
>>>    	return !memchr_inv(edid, 0, EDID_LENGTH);
>>>    }
>>>    
>>> -/**
>>> - * drm_edid_are_equal - compare two edid blobs.
>>> - * @edid1: pointer to first blob
>>> - * @edid2: pointer to second blob
>>> - * This helper can be used during probing to determine if
>>> - * edid had changed.
>>> - */
>>> -static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
>>> +static bool drm_edid_eq(const struct drm_edid *drm_edid,
>>> +			const void *raw_edid, size_t raw_edid_size)
>>>    {
>>> -	int edid1_len, edid2_len;
>>> -	bool edid1_present = edid1 != NULL;
>>> -	bool edid2_present = edid2 != NULL;
>>> +	bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
>>> +	bool edid2_present = raw_edid && raw_edid_size;
>>>    
>>>    	if (edid1_present != edid2_present)
>>>    		return false;
>>>    
>>> -	if (edid1) {
>>> -		edid1_len = edid_size(edid1);
>>> -		edid2_len = edid_size(edid2);
>>> -
>>> -		if (edid1_len != edid2_len)
>>> +	if (edid1_present) {
>>> +		if (drm_edid->size != raw_edid_size)
>>>    			return false;
>>>    
>>> -		if (memcmp(edid1, edid2, edid1_len))
>>> +		if (memcmp(drm_edid->edid, raw_edid, drm_edid->size))
>>>    			return false;
>>>    	}
>>>    
>>> @@ -6936,15 +6926,14 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
>>>    	int ret;
>>>    
>>>    	if (connector->edid_blob_ptr) {
>>> -		const struct edid *old_edid = connector->edid_blob_ptr->data;
>>> -
>>> -		if (old_edid) {
>>> -			if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) {
>>> -				connector->epoch_counter++;
>>> -				drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
>>> -					    connector->base.id, connector->name,
>>> -					    connector->epoch_counter);
>>> -			}
>>> +		const void *old_edid = connector->edid_blob_ptr->data;
>>> +		size_t old_edid_size = connector->edid_blob_ptr->length;
>>> +
>>> +		if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) {
>>> +			connector->epoch_counter++;
>>> +			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
>>> +				    connector->base.id, connector->name,
>>> +				    connector->epoch_counter);
>>>    		}
>>>    	}
>>>
Jani Nikula April 17, 2024, 8:21 a.m. UTC | #4
On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 16.04.24 um 14:27 schrieb Jani Nikula:
>> On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Hi
>>>
>>> Am 16.04.24 um 11:20 schrieb Jani Nikula:
>>>> Repurpose drm_edid_are_equal() to be more helpful for its single user,
>>>> and rename drm_edid_eq(). Functionally deduce the length from the blob
>>>> size, not the blob data, making it more robust against any errors.
>>> Could be squashed into patch 6.
>> Ack.
>>
>> Thanks for the review. I'll hold of on resending these until there are
>> some R-b's... I've send them a few times already with no comments. :(
>
> Feel free to add
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>
> to the series.

Many thanks! Just to double check, do you want me to move patch 5
earlier and squash patches 6&7?

BR,
Jani.
Thomas Zimmermann April 17, 2024, 11:13 a.m. UTC | #5
Hi

Am 17.04.24 um 10:21 schrieb Jani Nikula:
> On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 16.04.24 um 14:27 schrieb Jani Nikula:
>>> On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Hi
>>>>
>>>> Am 16.04.24 um 11:20 schrieb Jani Nikula:
>>>>> Repurpose drm_edid_are_equal() to be more helpful for its single user,
>>>>> and rename drm_edid_eq(). Functionally deduce the length from the blob
>>>>> size, not the blob data, making it more robust against any errors.
>>>> Could be squashed into patch 6.
>>> Ack.
>>>
>>> Thanks for the review. I'll hold of on resending these until there are
>>> some R-b's... I've send them a few times already with no comments. :(
>> Feel free to add
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> to the series.
> Many thanks! Just to double check, do you want me to move patch 5
> earlier and squash patches 6&7?

Your choice. Either is fine by me.

Best regards
Thomas

>
> BR,
> Jani.
>
>
Jani Nikula April 17, 2024, 3:20 p.m. UTC | #6
On Wed, 17 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Many thanks! Just to double check, do you want me to move patch 5
>> earlier and squash patches 6&7?
>
> Your choice. Either is fine by me.

I jumped at the easy option and merged this as-is. :)

Thanks again,
Jani.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 463fbad85d90..513590931cc5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1820,30 +1820,20 @@  static bool edid_block_is_zero(const void *edid)
 	return !memchr_inv(edid, 0, EDID_LENGTH);
 }
 
-/**
- * drm_edid_are_equal - compare two edid blobs.
- * @edid1: pointer to first blob
- * @edid2: pointer to second blob
- * This helper can be used during probing to determine if
- * edid had changed.
- */
-static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
+static bool drm_edid_eq(const struct drm_edid *drm_edid,
+			const void *raw_edid, size_t raw_edid_size)
 {
-	int edid1_len, edid2_len;
-	bool edid1_present = edid1 != NULL;
-	bool edid2_present = edid2 != NULL;
+	bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
+	bool edid2_present = raw_edid && raw_edid_size;
 
 	if (edid1_present != edid2_present)
 		return false;
 
-	if (edid1) {
-		edid1_len = edid_size(edid1);
-		edid2_len = edid_size(edid2);
-
-		if (edid1_len != edid2_len)
+	if (edid1_present) {
+		if (drm_edid->size != raw_edid_size)
 			return false;
 
-		if (memcmp(edid1, edid2, edid1_len))
+		if (memcmp(drm_edid->edid, raw_edid, drm_edid->size))
 			return false;
 	}
 
@@ -6936,15 +6926,14 @@  static int _drm_edid_connector_property_update(struct drm_connector *connector,
 	int ret;
 
 	if (connector->edid_blob_ptr) {
-		const struct edid *old_edid = connector->edid_blob_ptr->data;
-
-		if (old_edid) {
-			if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) {
-				connector->epoch_counter++;
-				drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
-					    connector->base.id, connector->name,
-					    connector->epoch_counter);
-			}
+		const void *old_edid = connector->edid_blob_ptr->data;
+		size_t old_edid_size = connector->edid_blob_ptr->length;
+
+		if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) {
+			connector->epoch_counter++;
+			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
+				    connector->base.id, connector->name,
+				    connector->epoch_counter);
 		}
 	}