[PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[Jani Nikula]

This is just the first two patches of a lengthy series that I'm not
really sure how to proceed with. Basically the series converts all of
drm/bridge to the new struct drm_edid infrastructure. It's safer than
struct edid, because it contains meta information about the allocated
size of the EDID, instead of relying on the size (number of extensions)
originating from outside of the kernel.

The rest is at [1]. The commit messages are lacking, and I don't really
have the toolchain to even build test most of it. But I think this is
where drm/bridge should go. Among all of drm, I think bridge has the
most uses of struct edid that do not originate from the drm_get_edid()
family of functions, which means the validity checks are somewhat
inconsistent, and having the meta information is more crucial.

Bridge maintainers, please instruct how to best proceed with this.


Thanks,
Jani.



[1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge



Jani Nikula (2):
  drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
  drm/bridge: switch to drm_bridge_read_edid()

 drivers/gpu/drm/drm_bridge.c           | 46 +++++++++++++++++++++++++-
 drivers/gpu/drm/drm_bridge_connector.c | 16 ++++-----
 include/drm/drm_bridge.h               | 33 ++++++++++++++++++
 3 files changed, 86 insertions(+), 9 deletions(-)

-- 
2.39.2

[PATCH 1/2] drm/bridge: add ->edid_read hook and drm_bridge_edid_read()

[Jani Nikula]

Add new struct drm_edid based ->edid_read hook and
drm_bridge_edid_read() function to call the hook.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_bridge.c | 46 +++++++++++++++++++++++++++++++++++-
 include/drm/drm_bridge.h     | 33 ++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 30d66bee0ec6..e1cfba2ff583 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -27,8 +27,9 @@
 #include <linux/mutex.h>
 
 #include <drm/drm_atomic_state_helper.h>
-#include <drm/drm_debugfs.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_debugfs.h>
+#include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_file.h>
 #include <drm/drm_of.h>
@@ -1206,6 +1207,47 @@ int drm_bridge_get_modes(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL_GPL(drm_bridge_get_modes);
 
+/**
+ * drm_bridge_edid_read - read the EDID data of the connected display
+ * @bridge: bridge control structure
+ * @connector: the connector to read EDID for
+ *
+ * If the bridge supports output EDID retrieval, as reported by the
+ * DRM_BRIDGE_OP_EDID bridge ops flag, call &drm_bridge_funcs.edid_read to get
+ * the EDID and return it. Otherwise return NULL.
+ *
+ * If &drm_bridge_funcs.edid_read is not set, fall back to using
+ * drm_bridge_get_edid() and wrapping it in struct drm_edid.
+ *
+ * RETURNS:
+ * The retrieved EDID on success, or NULL otherwise.
+ */
+const struct drm_edid *drm_bridge_edid_read(struct drm_bridge *bridge,
+					    struct drm_connector *connector)
+{
+	if (!(bridge->ops & DRM_BRIDGE_OP_EDID))
+		return NULL;
+
+	/* Transitional: Fall back to ->get_edid. */
+	if (!bridge->funcs->edid_read) {
+		const struct drm_edid *drm_edid;
+		struct edid *edid;
+
+		edid = drm_bridge_get_edid(bridge, connector);
+		if (!edid)
+			return NULL;
+
+		drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
+
+		kfree(edid);
+
+		return drm_edid;
+	}
+
+	return bridge->funcs->edid_read(bridge, connector);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_edid_read);
+
 /**
  * drm_bridge_get_edid - get the EDID data of the connected display
  * @bridge: bridge control structure
@@ -1215,6 +1257,8 @@ EXPORT_SYMBOL_GPL(drm_bridge_get_modes);
  * DRM_BRIDGE_OP_EDID bridge ops flag, call &drm_bridge_funcs.get_edid to
  * get the EDID and return it. Otherwise return NULL.
  *
+ * Deprecated. Prefer using drm_bridge_edid_read().
+ *
  * RETURNS:
  * The retrieved EDID on success, or NULL otherwise.
  */
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index cfb7dcdb66c4..1a8ba92c889c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -557,6 +557,37 @@ struct drm_bridge_funcs {
 	int (*get_modes)(struct drm_bridge *bridge,
 			 struct drm_connector *connector);
 
+	/**
+	 * @edid_read:
+	 *
+	 * Read the EDID data of the connected display.
+	 *
+	 * The @edid_read callback is the preferred way of reporting mode
+	 * information for a display connected to the bridge output. Bridges
+	 * that support reading EDID shall implement this callback and leave
+	 * the @get_modes callback unimplemented.
+	 *
+	 * The caller of this operation shall first verify the output
+	 * connection status and refrain from reading EDID from a disconnected
+	 * output.
+	 *
+	 * This callback is optional. Bridges that implement it shall set the
+	 * DRM_BRIDGE_OP_EDID flag in their &drm_bridge->ops.
+	 *
+	 * The connector parameter shall be used for the sole purpose of EDID
+	 * retrieval, and shall not be stored internally by bridge drivers for
+	 * future usage.
+	 *
+	 * RETURNS:
+	 *
+	 * An edid structure newly allocated with drm_edid_alloc() or returned
+	 * from drm_edid_read() family of functions on success, or NULL
+	 * otherwise. The caller is responsible for freeing the returned edid
+	 * structure with drm_edid_free().
+	 */
+	const struct drm_edid *(*edid_read)(struct drm_bridge *bridge,
+					    struct drm_connector *connector);
+
 	/**
 	 * @get_edid:
 	 *
@@ -888,6 +919,8 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,
 enum drm_connector_status drm_bridge_detect(struct drm_bridge *bridge);
 int drm_bridge_get_modes(struct drm_bridge *bridge,
 			 struct drm_connector *connector);
+const struct drm_edid *drm_bridge_edid_read(struct drm_bridge *bridge,
+					    struct drm_connector *connector);
 struct edid *drm_bridge_get_edid(struct drm_bridge *bridge,
 				 struct drm_connector *connector);
 void drm_bridge_hpd_enable(struct drm_bridge *bridge,
-- 
2.39.2

[PATCH 2/2] drm/bridge: switch to drm_bridge_read_edid()

[Jani Nikula]

Prefer using the struct drm_edid based functions.

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

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 8239ad43aed5..b7d7092aad9f 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -245,27 +245,27 @@ static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
 					       struct drm_bridge *bridge)
 {
 	enum drm_connector_status status;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
 	int n;
 
 	status = drm_bridge_connector_detect(connector, false);
 	if (status != connector_status_connected)
 		goto no_edid;
 
-	edid = drm_bridge_get_edid(bridge, connector);
-	if (!drm_edid_is_valid(edid)) {
-		kfree(edid);
+	drm_edid = drm_bridge_edid_read(bridge, connector);
+	if (!drm_edid_valid(drm_edid)) {
+		drm_edid_free(drm_edid);
 		goto no_edid;
 	}
 
-	drm_connector_update_edid_property(connector, edid);
-	n = drm_add_edid_modes(connector, edid);
+	drm_edid_connector_update(connector, drm_edid);
+	n = drm_edid_connector_add_modes(connector);
 
-	kfree(edid);
+	drm_edid_free(drm_edid);
 	return n;
 
 no_edid:
-	drm_connector_update_edid_property(connector, NULL);
+	drm_edid_connector_update(connector, NULL);
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[Jani Nikula]

On Thu, 26 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> This is just the first two patches of a lengthy series that I'm not
> really sure how to proceed with. Basically the series converts all of
> drm/bridge to the new struct drm_edid infrastructure. It's safer than
> struct edid, because it contains meta information about the allocated
> size of the EDID, instead of relying on the size (number of extensions)
> originating from outside of the kernel.
>
> The rest is at [1]. The commit messages are lacking, and I don't really
> have the toolchain to even build test most of it. But I think this is
> where drm/bridge should go. Among all of drm, I think bridge has the
> most uses of struct edid that do not originate from the drm_get_edid()
> family of functions, which means the validity checks are somewhat
> inconsistent, and having the meta information is more crucial.
>
> Bridge maintainers, please instruct how to best proceed with this.

Ping.

The two patches posted here could be merged, to add something to build
the later commits on gradually.

BR,
Jani.

>
>
> Thanks,
> Jani.
>
>
>
> [1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
>
>
>
> Jani Nikula (2):
>   drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
>   drm/bridge: switch to drm_bridge_read_edid()
>
>  drivers/gpu/drm/drm_bridge.c           | 46 +++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_bridge_connector.c | 16 ++++-----
>  include/drm/drm_bridge.h               | 33 ++++++++++++++++++
>  3 files changed, 86 insertions(+), 9 deletions(-)

-- 
Jani Nikula, Intel

Re: [PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[Jani Nikula]

On Tue, 14 Nov 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 26 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>> This is just the first two patches of a lengthy series that I'm not
>> really sure how to proceed with. Basically the series converts all of
>> drm/bridge to the new struct drm_edid infrastructure. It's safer than
>> struct edid, because it contains meta information about the allocated
>> size of the EDID, instead of relying on the size (number of extensions)
>> originating from outside of the kernel.
>>
>> The rest is at [1]. The commit messages are lacking, and I don't really
>> have the toolchain to even build test most of it. But I think this is
>> where drm/bridge should go. Among all of drm, I think bridge has the
>> most uses of struct edid that do not originate from the drm_get_edid()
>> family of functions, which means the validity checks are somewhat
>> inconsistent, and having the meta information is more crucial.
>>
>> Bridge maintainers, please instruct how to best proceed with this.
>
> Ping.

Ping.

>
> The two patches posted here could be merged, to add something to build
> the later commits on gradually.
>
> BR,
> Jani.
>
>>
>>
>> Thanks,
>> Jani.
>>
>>
>>
>> [1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
>>
>>
>>
>> Jani Nikula (2):
>>   drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
>>   drm/bridge: switch to drm_bridge_read_edid()
>>
>>  drivers/gpu/drm/drm_bridge.c           | 46 +++++++++++++++++++++++++-
>>  drivers/gpu/drm/drm_bridge_connector.c | 16 ++++-----
>>  include/drm/drm_bridge.h               | 33 ++++++++++++++++++
>>  3 files changed, 86 insertions(+), 9 deletions(-)

-- 
Jani Nikula, Intel

Re: [PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[Neil Armstrong]

On 19/12/2023 13:15, Jani Nikula wrote:
> On Tue, 14 Nov 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>> On Thu, 26 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>>> This is just the first two patches of a lengthy series that I'm not
>>> really sure how to proceed with. Basically the series converts all of
>>> drm/bridge to the new struct drm_edid infrastructure. It's safer than
>>> struct edid, because it contains meta information about the allocated
>>> size of the EDID, instead of relying on the size (number of extensions)
>>> originating from outside of the kernel.
>>>
>>> The rest is at [1]. The commit messages are lacking, and I don't really
>>> have the toolchain to even build test most of it. But I think this is
>>> where drm/bridge should go. Among all of drm, I think bridge has the
>>> most uses of struct edid that do not originate from the drm_get_edid()
>>> family of functions, which means the validity checks are somewhat
>>> inconsistent, and having the meta information is more crucial.
>>>
>>> Bridge maintainers, please instruct how to best proceed with this.
>>
>> Ping.
> 
> Ping.

Sorry for the delay, I would have preferred changing the get_edid to return
a drm_edid, but I understand the task is too high, could you instead use
@get_drm_edid instead of @edid_read ?

And perhaps convert one very common bridge to this so we can validate
the change in CI.

Neil

> 
>>
>> The two patches posted here could be merged, to add something to build
>> the later commits on gradually.
>>
>> BR,
>> Jani.
>>
>>>
>>>
>>> Thanks,
>>> Jani.
>>>
>>>
>>>
>>> [1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
>>>
>>>
>>>
>>> Jani Nikula (2):
>>>    drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
>>>    drm/bridge: switch to drm_bridge_read_edid()
>>>
>>>   drivers/gpu/drm/drm_bridge.c           | 46 +++++++++++++++++++++++++-
>>>   drivers/gpu/drm/drm_bridge_connector.c | 16 ++++-----
>>>   include/drm/drm_bridge.h               | 33 ++++++++++++++++++
>>>   3 files changed, 86 insertions(+), 9 deletions(-)
> 

Re: [PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[Jani Nikula]

On Fri, 22 Dec 2023, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> On 19/12/2023 13:15, Jani Nikula wrote:
>> On Tue, 14 Nov 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>>> On Thu, 26 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>>>> This is just the first two patches of a lengthy series that I'm not
>>>> really sure how to proceed with. Basically the series converts all of
>>>> drm/bridge to the new struct drm_edid infrastructure. It's safer than
>>>> struct edid, because it contains meta information about the allocated
>>>> size of the EDID, instead of relying on the size (number of extensions)
>>>> originating from outside of the kernel.
>>>>
>>>> The rest is at [1]. The commit messages are lacking, and I don't really
>>>> have the toolchain to even build test most of it. But I think this is
>>>> where drm/bridge should go. Among all of drm, I think bridge has the
>>>> most uses of struct edid that do not originate from the drm_get_edid()
>>>> family of functions, which means the validity checks are somewhat
>>>> inconsistent, and having the meta information is more crucial.
>>>>
>>>> Bridge maintainers, please instruct how to best proceed with this.
>>>
>>> Ping.
>> 
>> Ping.
>
> Sorry for the delay, I would have preferred changing the get_edid to return
> a drm_edid, but I understand the task is too high, could you instead use
> @get_drm_edid instead of @edid_read ?

edid_read matches the changes in drm_edid.c, going from drm_get_edid()
to drm_edid_read().

There's a nice symmetry when ->get_edid() hooks using drm_get_edid() are
converted to ->edid_read() hooks using drm_edid_read().

> And perhaps convert one very common bridge to this so we can validate
> the change in CI.

So I did convert all of bridge over a few months back, and pushed the
branch to [1]. Should I brush that up and send the entire series? I
don't really know what's common and what's not.


BR,
Jani.

>
> Neil
>
>> 
>>>
>>> The two patches posted here could be merged, to add something to build
>>> the later commits on gradually.
>>>
>>> BR,
>>> Jani.
>>>
>>>>
>>>>
>>>> Thanks,
>>>> Jani.
>>>>
>>>>
>>>>
>>>> [1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
>>>>
>>>>
>>>>
>>>> Jani Nikula (2):
>>>>    drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
>>>>    drm/bridge: switch to drm_bridge_read_edid()
>>>>
>>>>   drivers/gpu/drm/drm_bridge.c           | 46 +++++++++++++++++++++++++-
>>>>   drivers/gpu/drm/drm_bridge_connector.c | 16 ++++-----
>>>>   include/drm/drm_bridge.h               | 33 ++++++++++++++++++
>>>>   3 files changed, 86 insertions(+), 9 deletions(-)
>> 
>

-- 
Jani Nikula, Intel

Re: [PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[Jani Nikula]

On Fri, 22 Dec 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> On Fri, 22 Dec 2023, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>> On 19/12/2023 13:15, Jani Nikula wrote:
>>> On Tue, 14 Nov 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>>>> On Thu, 26 Oct 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>>>>> This is just the first two patches of a lengthy series that I'm not
>>>>> really sure how to proceed with. Basically the series converts all of
>>>>> drm/bridge to the new struct drm_edid infrastructure. It's safer than
>>>>> struct edid, because it contains meta information about the allocated
>>>>> size of the EDID, instead of relying on the size (number of extensions)
>>>>> originating from outside of the kernel.
>>>>>
>>>>> The rest is at [1]. The commit messages are lacking, and I don't really
>>>>> have the toolchain to even build test most of it. But I think this is
>>>>> where drm/bridge should go. Among all of drm, I think bridge has the
>>>>> most uses of struct edid that do not originate from the drm_get_edid()
>>>>> family of functions, which means the validity checks are somewhat
>>>>> inconsistent, and having the meta information is more crucial.
>>>>>
>>>>> Bridge maintainers, please instruct how to best proceed with this.
>>>>
>>>> Ping.
>>> 
>>> Ping.
>>
>> Sorry for the delay, I would have preferred changing the get_edid to return
>> a drm_edid, but I understand the task is too high, could you instead use
>> @get_drm_edid instead of @edid_read ?
>
> edid_read matches the changes in drm_edid.c, going from drm_get_edid()
> to drm_edid_read().
>
> There's a nice symmetry when ->get_edid() hooks using drm_get_edid() are
> converted to ->edid_read() hooks using drm_edid_read().
>
>> And perhaps convert one very common bridge to this so we can validate
>> the change in CI.
>
> So I did convert all of bridge over a few months back, and pushed the
> branch to [1]. Should I brush that up and send the entire series? I
> don't really know what's common and what's not.

Okay, I rebased and pushed [1]. Probably doesn't make sense to send a
patch bomb like that right now...

BR,
Jani.


[1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge


-- 
Jani Nikula, Intel

Re: [PATCH 1/2] drm/bridge: add ->edid_read hook and drm_bridge_edid_read()

[Dmitry Baryshkov]

On Thu, 26 Oct 2023 at 12:40, Jani Nikula <jani.nikula@intel.com> wrote:
>
> Add new struct drm_edid based ->edid_read hook and
> drm_bridge_edid_read() function to call the hook.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 46 +++++++++++++++++++++++++++++++++++-
>  include/drm/drm_bridge.h     | 33 ++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 30d66bee0ec6..e1cfba2ff583 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -27,8 +27,9 @@
>  #include <linux/mutex.h>
>
>  #include <drm/drm_atomic_state_helper.h>
> -#include <drm/drm_debugfs.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_debugfs.h>
> +#include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_of.h>
> @@ -1206,6 +1207,47 @@ int drm_bridge_get_modes(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL_GPL(drm_bridge_get_modes);
>
> +/**
> + * drm_bridge_edid_read - read the EDID data of the connected display
> + * @bridge: bridge control structure
> + * @connector: the connector to read EDID for
> + *
> + * If the bridge supports output EDID retrieval, as reported by the
> + * DRM_BRIDGE_OP_EDID bridge ops flag, call &drm_bridge_funcs.edid_read to get
> + * the EDID and return it. Otherwise return NULL.
> + *
> + * If &drm_bridge_funcs.edid_read is not set, fall back to using
> + * drm_bridge_get_edid() and wrapping it in struct drm_edid.
> + *
> + * RETURNS:
> + * The retrieved EDID on success, or NULL otherwise.

Wouldn't it be better to return an ERR_PTR instead of NULL?

> + */
> +const struct drm_edid *drm_bridge_edid_read(struct drm_bridge *bridge,
> +                                           struct drm_connector *connector)
> +{
> +       if (!(bridge->ops & DRM_BRIDGE_OP_EDID))
> +               return NULL;
> +
> +       /* Transitional: Fall back to ->get_edid. */
> +       if (!bridge->funcs->edid_read) {
> +               const struct drm_edid *drm_edid;
> +               struct edid *edid;
> +
> +               edid = drm_bridge_get_edid(bridge, connector);
> +               if (!edid)
> +                       return NULL;
> +
> +               drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
> +
> +               kfree(edid);
> +
> +               return drm_edid;
> +       }
> +
> +       return bridge->funcs->edid_read(bridge, connector);
> +}
> +EXPORT_SYMBOL_GPL(drm_bridge_edid_read);

[skipped the rest]

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm/bridge: add ->edid_read hook and drm_bridge_edid_read()

[Jani Nikula]

On Fri, 22 Dec 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Thu, 26 Oct 2023 at 12:40, Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> Add new struct drm_edid based ->edid_read hook and
>> drm_bridge_edid_read() function to call the hook.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 46 +++++++++++++++++++++++++++++++++++-
>>  include/drm/drm_bridge.h     | 33 ++++++++++++++++++++++++++
>>  2 files changed, 78 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 30d66bee0ec6..e1cfba2ff583 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -27,8 +27,9 @@
>>  #include <linux/mutex.h>
>>
>>  #include <drm/drm_atomic_state_helper.h>
>> -#include <drm/drm_debugfs.h>
>>  #include <drm/drm_bridge.h>
>> +#include <drm/drm_debugfs.h>
>> +#include <drm/drm_edid.h>
>>  #include <drm/drm_encoder.h>
>>  #include <drm/drm_file.h>
>>  #include <drm/drm_of.h>
>> @@ -1206,6 +1207,47 @@ int drm_bridge_get_modes(struct drm_bridge *bridge,
>>  }
>>  EXPORT_SYMBOL_GPL(drm_bridge_get_modes);
>>
>> +/**
>> + * drm_bridge_edid_read - read the EDID data of the connected display
>> + * @bridge: bridge control structure
>> + * @connector: the connector to read EDID for
>> + *
>> + * If the bridge supports output EDID retrieval, as reported by the
>> + * DRM_BRIDGE_OP_EDID bridge ops flag, call &drm_bridge_funcs.edid_read to get
>> + * the EDID and return it. Otherwise return NULL.
>> + *
>> + * If &drm_bridge_funcs.edid_read is not set, fall back to using
>> + * drm_bridge_get_edid() and wrapping it in struct drm_edid.
>> + *
>> + * RETURNS:
>> + * The retrieved EDID on success, or NULL otherwise.
>
> Wouldn't it be better to return an ERR_PTR instead of NULL?

I don't think so. The existing drm_bridge_get_edid() returns NULL on
errors. The ->get_edid hook returns NULL on errors. The new ->edid_read
returns NULL on errors. The drm_get_edid() and drm_edid_read() functions
return NULL on errors.

It would be surprising if one of the functions started returning error
pointers.

I don't see any added benefit with error pointers here. The ->edid_read
hook could be made to return error pointers too, but then there's quite
the burden in making all drivers return sensible values where the
difference in error code actually matters. And which error scenarios do
we want to differentiate here? How should we handle them differently?


BR,
Jani.


>
>> + */
>> +const struct drm_edid *drm_bridge_edid_read(struct drm_bridge *bridge,
>> +                                           struct drm_connector *connector)
>> +{
>> +       if (!(bridge->ops & DRM_BRIDGE_OP_EDID))
>> +               return NULL;
>> +
>> +       /* Transitional: Fall back to ->get_edid. */
>> +       if (!bridge->funcs->edid_read) {
>> +               const struct drm_edid *drm_edid;
>> +               struct edid *edid;
>> +
>> +               edid = drm_bridge_get_edid(bridge, connector);
>> +               if (!edid)
>> +                       return NULL;
>> +
>> +               drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
>> +
>> +               kfree(edid);
>> +
>> +               return drm_edid;
>> +       }
>> +
>> +       return bridge->funcs->edid_read(bridge, connector);
>> +}
>> +EXPORT_SYMBOL_GPL(drm_bridge_edid_read);
>
> [skipped the rest]

-- 
Jani Nikula, Intel

Re: [PATCH 0/2] drm/bridge: start moving towards struct drm_edid

[Jani Nikula]

On Fri, 22 Dec 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> Okay, I rebased and pushed [1]. Probably doesn't make sense to send a
> patch bomb like that right now...

And here are the patches:

https://patchwork.freedesktop.org/series/128149/

BR,
Jani.


>
> BR,
> Jani.
>
>
> [1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge

-- 
Jani Nikula, Intel