[REBASE 0/7] drm/edid: cleanups, rebase

[REBASE 0/7] drm/edid: cleanups, rebase

[Jani Nikula]

Rebase.

Jani Nikula (7):
  drm/displayid: move drm_displayid.h to drm_displayd_internal.h
  drm/edid: move all internal declarations to drm_crtc_internal.h
  drm/edid: group struct drm_edid based declarations together
  drm/edid: rename drm_find_edid_extension() to
    drm_edid_find_extension()
  drm/edid: avoid drm_edid_find_extension() internally
  drm/edid: make drm_edid_are_equal() static
  drm/edid: make drm_edid_are_equal() more convenient for its single
    user

 drivers/gpu/drm/drm_crtc_internal.h           |  6 ++
 drivers/gpu/drm/drm_displayid.c               |  7 ++-
 .../gpu/drm/drm_displayid_internal.h          |  5 +-
 drivers/gpu/drm/drm_edid.c                    | 60 +++++++++----------
 drivers/gpu/drm/drm_eld.c                     |  4 +-
 drivers/gpu/drm/drm_internal.h                |  5 --
 include/drm/drm_edid.h                        | 13 ++--
 7 files changed, 50 insertions(+), 50 deletions(-)
 rename include/drm/drm_displayid.h => drivers/gpu/drm/drm_displayid_internal.h (98%)

-- 
2.39.2

[REBASE 1/7] drm/displayid: move drm_displayid.h to drm_displayd_internal.h

[Jani Nikula]

There are no exported symbols for displayid, and it's all internal
interfaces. Move the header to drivers/gpu/drm/drm_displayd_internal.h.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_displayid.c                              | 3 ++-
 .../gpu/drm/drm_displayid_internal.h                         | 5 +++--
 drivers/gpu/drm/drm_edid.c                                   | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)
 rename include/drm/drm_displayid.h => drivers/gpu/drm/drm_displayid_internal.h (98%)

diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index 9edc111be7ee..f72a893c7deb 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -3,10 +3,11 @@
  * Copyright © 2021 Intel Corporation
  */
 
-#include <drm/drm_displayid.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_print.h>
 
+#include "drm_displayid_internal.h"
+
 static const struct displayid_header *
 displayid_get_header(const u8 *displayid, int length, int index)
 {
diff --git a/include/drm/drm_displayid.h b/drivers/gpu/drm/drm_displayid_internal.h
similarity index 98%
rename from include/drm/drm_displayid.h
rename to drivers/gpu/drm/drm_displayid_internal.h
index 566497eeb3b8..56fd3bb0a779 100644
--- a/include/drm/drm_displayid.h
+++ b/drivers/gpu/drm/drm_displayid_internal.h
@@ -19,8 +19,9 @@
  * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
  * OTHER DEALINGS IN THE SOFTWARE.
  */
-#ifndef DRM_DISPLAYID_H
-#define DRM_DISPLAYID_H
+
+#ifndef __DRM_DISPLAYID_INTERNAL_H__
+#define __DRM_DISPLAYID_INTERNAL_H__
 
 #include <linux/types.h>
 #include <linux/bits.h>
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1400722ae3fe..c4f799059522 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -40,7 +40,6 @@
 #include <linux/slab.h>
 #include <linux/vga_switcheroo.h>
 
-#include <drm/drm_displayid.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_eld.h>
@@ -48,6 +47,7 @@
 #include <drm/drm_print.h>
 
 #include "drm_crtc_internal.h"
+#include "drm_displayid_internal.h"
 #include "drm_internal.h"
 
 static int oui(u8 first, u8 second, u8 third)
-- 
2.39.2

[REBASE 2/7] drm/edid: move all internal declarations to drm_crtc_internal.h

[Jani Nikula]

The declarations for internal EDID functions are a bit scattered. Put
them all in drm_crtc_internal.h.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h | 6 ++++++
 drivers/gpu/drm/drm_displayid.c     | 1 +
 drivers/gpu/drm/drm_eld.c           | 4 +++-
 drivers/gpu/drm/drm_internal.h      | 5 -----
 include/drm/drm_edid.h              | 3 ---
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 0c693229a1c9..2256893d7d67 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -43,12 +43,14 @@ enum drm_color_range;
 enum drm_connector_force;
 enum drm_mode_status;
 
+struct cea_sad;
 struct drm_atomic_state;
 struct drm_bridge;
 struct drm_connector;
 struct drm_crtc;
 struct drm_device;
 struct drm_display_mode;
+struct drm_edid;
 struct drm_file;
 struct drm_framebuffer;
 struct drm_mode_create_dumb;
@@ -297,6 +299,10 @@ void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
 int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
 int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
 int drm_edid_override_reset(struct drm_connector *connector);
+const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
+				  int ext_id, int *ext_index);
+void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad);
+void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad);
 
 /* drm_edid_load.c */
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index f72a893c7deb..f800dc0906d5 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -6,6 +6,7 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_print.h>
 
+#include "drm_crtc_internal.h"
 #include "drm_displayid_internal.h"
 
 static const struct displayid_header *
diff --git a/drivers/gpu/drm/drm_eld.c b/drivers/gpu/drm/drm_eld.c
index 5177991aa272..c0428d07de53 100644
--- a/drivers/gpu/drm/drm_eld.c
+++ b/drivers/gpu/drm/drm_eld.c
@@ -3,10 +3,12 @@
  * Copyright © 2023 Intel Corporation
  */
 
+#include <linux/export.h>
+
 #include <drm/drm_edid.h>
 #include <drm/drm_eld.h>
 
-#include "drm_internal.h"
+#include "drm_crtc_internal.h"
 
 /**
  * drm_eld_sad_get - get SAD from ELD to struct cea_sad
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 2215baef9a3e..690505a1f7a5 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -35,7 +35,6 @@
 
 #define DRM_IF_VERSION(maj, min) (maj << 16 | min)
 
-struct cea_sad;
 struct dentry;
 struct dma_buf;
 struct iosys_map;
@@ -278,8 +277,4 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
 				const struct drm_framebuffer *fb);
 void drm_framebuffer_debugfs_init(struct drm_device *dev);
 
-/* drm_edid.c */
-void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad);
-void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad);
-
 #endif /* __DRM_INTERNAL_H__ */
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 8e0e32349332..324e900cc287 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -485,7 +485,4 @@ void drm_edid_get_product_id(const struct drm_edid *drm_edid,
 void drm_edid_print_product_id(struct drm_printer *p,
 			       const struct drm_edid_product_id *id, bool raw);
 
-const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
-				  int ext_id, int *ext_index);
-
 #endif /* __DRM_EDID_H__ */
-- 
2.39.2

[REBASE 3/7] drm/edid: group struct drm_edid based declarations together

[Jani Nikula]

Keep the declarations for struct drm_edid based functions together.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 include/drm/drm_edid.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 324e900cc287..92fff199aa78 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -431,10 +431,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	void *data);
 struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter);
-const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
-u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
-bool drm_edid_match(const struct drm_edid *drm_edid,
-		    const struct drm_edid_ident *ident);
 struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
 struct edid *drm_edid_duplicate(const struct edid *edid);
@@ -474,6 +470,7 @@ const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
 const struct drm_edid *drm_edid_read_custom(struct drm_connector *connector,
 					    int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
 					    void *context);
+const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
 const struct drm_edid *drm_edid_read_switcheroo(struct drm_connector *connector,
 						struct i2c_adapter *adapter);
 int drm_edid_connector_update(struct drm_connector *connector,
@@ -484,5 +481,8 @@ void drm_edid_get_product_id(const struct drm_edid *drm_edid,
 			     struct drm_edid_product_id *id);
 void drm_edid_print_product_id(struct drm_printer *p,
 			       const struct drm_edid_product_id *id, bool raw);
+u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
+bool drm_edid_match(const struct drm_edid *drm_edid,
+		    const struct drm_edid_ident *ident);
 
 #endif /* __DRM_EDID_H__ */
-- 
2.39.2

[REBASE 4/7] drm/edid: rename drm_find_edid_extension() to drm_edid_find_extension()

[Jani Nikula]

Follow the drm_edid_ naming convention.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h | 2 +-
 drivers/gpu/drm/drm_displayid.c     | 3 ++-
 drivers/gpu/drm/drm_edid.c          | 4 ++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 2256893d7d67..25aaae937ceb 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -299,7 +299,7 @@ void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
 int drm_edid_override_show(struct drm_connector *connector, struct seq_file *m);
 int drm_edid_override_set(struct drm_connector *connector, const void *edid, size_t size);
 int drm_edid_override_reset(struct drm_connector *connector);
-const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
+const u8 *drm_edid_find_extension(const struct drm_edid *drm_edid,
 				  int ext_id, int *ext_index);
 void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad);
 void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad);
diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c
index f800dc0906d5..9d01d762801f 100644
--- a/drivers/gpu/drm/drm_displayid.c
+++ b/drivers/gpu/drm/drm_displayid.c
@@ -55,9 +55,10 @@ static const u8 *drm_find_displayid_extension(const struct drm_edid *drm_edid,
 					      int *length, int *idx,
 					      int *ext_index)
 {
-	const u8 *displayid = drm_find_edid_extension(drm_edid, DISPLAYID_EXT, ext_index);
 	const struct displayid_header *base;
+	const u8 *displayid;
 
+	displayid = drm_edid_find_extension(drm_edid, DISPLAYID_EXT, ext_index);
 	if (!displayid)
 		return NULL;
 
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c4f799059522..c29f31dcc818 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4200,7 +4200,7 @@ static int add_detailed_modes(struct drm_connector *connector,
  *
  * FIXME: Prefer not returning pointers to raw EDID data.
  */
-const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
+const u8 *drm_edid_find_extension(const struct drm_edid *drm_edid,
 				  int ext_id, int *ext_index)
 {
 	const u8 *edid_ext = NULL;
@@ -4234,7 +4234,7 @@ static bool drm_edid_has_cta_extension(const struct drm_edid *drm_edid)
 	bool found = false;
 
 	/* Look for a top level CEA extension block */
-	if (drm_find_edid_extension(drm_edid, CEA_EXT, &ext_index))
+	if (drm_edid_find_extension(drm_edid, CEA_EXT, &ext_index))
 		return true;
 
 	/* CEA blocks can also be found embedded in a DisplayID block */
-- 
2.39.2

[REBASE 5/7] drm/edid: avoid drm_edid_find_extension() internally

[Jani Nikula]

Prefer the EDID iterators over drm_edid_find_extension() in
drm_edid_has_cta_extension(), even if this leads to more code. The key
is to use the same patterns as much as possible.

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c29f31dcc818..4b3ad42a8f95 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4230,11 +4230,21 @@ static bool drm_edid_has_cta_extension(const struct drm_edid *drm_edid)
 {
 	const struct displayid_block *block;
 	struct displayid_iter iter;
-	int ext_index = 0;
+	struct drm_edid_iter edid_iter;
+	const u8 *ext;
 	bool found = false;
 
 	/* Look for a top level CEA extension block */
-	if (drm_edid_find_extension(drm_edid, CEA_EXT, &ext_index))
+	drm_edid_iter_begin(drm_edid, &edid_iter);
+	drm_edid_iter_for_each(ext, &edid_iter) {
+		if (ext[0] == CEA_EXT) {
+			found = true;
+			break;
+		}
+	}
+	drm_edid_iter_end(&edid_iter);
+
+	if (found)
 		return true;
 
 	/* CEA blocks can also be found embedded in a DisplayID block */
-- 
2.39.2

Re: [REBASE 5/7] drm/edid: avoid drm_edid_find_extension() internally

[Thomas Zimmermann]

Hi

Am 16.04.24 um 11:19 schrieb Jani Nikula:
> Prefer the EDID iterators over drm_edid_find_extension() in
> drm_edid_has_cta_extension(), even if this leads to more code. The key
> is to use the same patterns as much as possible.

Should this patch go before patch 4? That would limit the impact of the 
latter.

Why is this instance different than the one in 
drm_find_displayid_extension()? Best regards Thomas
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/drm_edid.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c29f31dcc818..4b3ad42a8f95 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4230,11 +4230,21 @@ static bool drm_edid_has_cta_extension(const struct drm_edid *drm_edid)
>   {
>   	const struct displayid_block *block;
>   	struct displayid_iter iter;
> -	int ext_index = 0;
> +	struct drm_edid_iter edid_iter;
> +	const u8 *ext;
>   	bool found = false;
>   
>   	/* Look for a top level CEA extension block */
> -	if (drm_edid_find_extension(drm_edid, CEA_EXT, &ext_index))
> +	drm_edid_iter_begin(drm_edid, &edid_iter);
> +	drm_edid_iter_for_each(ext, &edid_iter) {
> +		if (ext[0] == CEA_EXT) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	drm_edid_iter_end(&edid_iter);
> +
> +	if (found)
>   		return true;
>   
>   	/* CEA blocks can also be found embedded in a DisplayID 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)

Re: [REBASE 5/7] drm/edid: avoid drm_edid_find_extension() internally

[Jani Nikula]

On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 16.04.24 um 11:19 schrieb Jani Nikula:
>> Prefer the EDID iterators over drm_edid_find_extension() in
>> drm_edid_has_cta_extension(), even if this leads to more code. The key
>> is to use the same patterns as much as possible.
>
> Should this patch go before patch 4? That would limit the impact of the 
> latter.

I can if you want, IMO not a big deal.

> Why is this instance different than the one in 
> drm_find_displayid_extension()? Best regards Thomas

Overall I'd like to get rid of the function altogether, but I'm
undecided what the replacement interface towards drm_displayid.c should
be. Maybe expose the drm_edid_iter_* stuff? But I really don't want
anyone to export and start using them in drivers.

BR,
Jani.

>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index c29f31dcc818..4b3ad42a8f95 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4230,11 +4230,21 @@ static bool drm_edid_has_cta_extension(const struct drm_edid *drm_edid)
>>   {
>>   	const struct displayid_block *block;
>>   	struct displayid_iter iter;
>> -	int ext_index = 0;
>> +	struct drm_edid_iter edid_iter;
>> +	const u8 *ext;
>>   	bool found = false;
>>   
>>   	/* Look for a top level CEA extension block */
>> -	if (drm_edid_find_extension(drm_edid, CEA_EXT, &ext_index))
>> +	drm_edid_iter_begin(drm_edid, &edid_iter);
>> +	drm_edid_iter_for_each(ext, &edid_iter) {
>> +		if (ext[0] == CEA_EXT) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +	drm_edid_iter_end(&edid_iter);
>> +
>> +	if (found)
>>   		return true;
>>   
>>   	/* CEA blocks can also be found embedded in a DisplayID block */

-- 
Jani Nikula, Intel

[REBASE 6/7] drm/edid: make drm_edid_are_equal() static

[Jani Nikula]

drm_edid_are_equal() is only used within drm_edid.c. Make it static. Do
not encourage more uses of struct edid.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 3 +--
 include/drm/drm_edid.h     | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 4b3ad42a8f95..463fbad85d90 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1827,7 +1827,7 @@ static bool edid_block_is_zero(const void *edid)
  * This helper can be used during probing to determine if
  * edid had changed.
  */
-bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
+static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
 {
 	int edid1_len, edid2_len;
 	bool edid1_present = edid1 != NULL;
@@ -1849,7 +1849,6 @@ bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2)
 
 	return true;
 }
-EXPORT_SYMBOL(drm_edid_are_equal);
 
 enum edid_block_status {
 	EDID_BLOCK_OK = 0,
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 92fff199aa78..b085525e53e2 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -348,8 +348,6 @@ int drm_edid_to_speaker_allocation(const struct edid *edid, u8 **sadb);
 int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
-bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2);
-
 int
 drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 					 const struct drm_connector *connector,
-- 
2.39.2

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

[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.

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);
 		}
 	}
 
-- 
2.39.2

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

[Thomas Zimmermann]

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);
>   		}
>   	}
>   

-- 
--
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)

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

[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. :(

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, Intel

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

[Thomas Zimmermann]

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);
>>>    		}
>>>    	}
>>>    

-- 
--
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)

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

[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?

BR,
Jani.


-- 
Jani Nikula, Intel

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

[Thomas Zimmermann]

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.
>
>

-- 
--
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)

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

[Jani Nikula]

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.



-- 
Jani Nikula, Intel