drm/bridge: start moving towards struct drm_edid
Subsystems (3)
cover drm/bridge: start moving towards struct drm_edid
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(-)
Signature · 1 line
2.39.2
PATCH 1/2 drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
Add new struct drm_edid based ->edid_read hook and drm_bridge_edid_read() function to call the hook.
--- drivers/gpu/drm/drm_bridge.c | 46 +++++++++++++++++++++++++++++++++++- include/drm/drm_bridge.h | 33 ++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_bridge.c +45 -1
@@ -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. */
include/drm/drm_bridge.h +33 -0
@@ -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,
Signature · 1 line
2.39.2
Re: [PATCH 1/2] drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
Quoted 33 lines · Jani Nikula
Add new struct
drm_edidbased ->edid_read hook anddrm_bridge_edid_read()function to call the hook.Signed-off-by Jani Nikula <[email protected]>--- drivers/gpu/drm/drm_bridge.c | 46 +++++++++++++++++++++++++++++++++++- include/drm/drm_bridge.h | 33 ++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_bridge.c +6
-1@@ -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
drivers/gpu/drm/drm_bridge.c int drm_bridge_get_modes(struct drm_bridge *bridge, · line 1223
Quoted 10 lines
drivers/gpu/drm/drm_bridge.c +10
-0+* +* 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?
Quoted 16 lines
drivers/gpu/drm/drm_bridge.c +16
-0+*/ +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 ; +
drivers/gpu/drm/drm_bridge.c int drm_bridge_get_modes(struct drm_bridge *bridge, · line 1249
Quoted 10 lines
drivers/gpu/drm/drm_bridge.c +10
-0+ 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]
Signature · 2 lines
With best wishes Dmitry
Re: [PATCH 1/2] drm/bridge: add ->edid_read hook and drm_bridge_edid_read()
Quoted 30 lines · Dmitry Baryshkov
Quoted 29 lines · Jani Nikula
Add new struct
drm_edidbased ->edid_read hook anddrm_bridge_edid_read()function to call the hook.Signed-off-by Jani Nikula <[email protected]>--- drivers/gpu/drm/drm_bridge.c | 46 +++++++++++++++++++++++++++++++++++- include/drm/drm_bridge.h | 33 ++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_bridge.c +2
-1@@ -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);
Quoted 16 lines
Quoted 4 lines
drivers/gpu/drm/drm_bridge.c +4
-0+/** +* drm_bridge_edid_read - read the EDID data of the connected display +* @bridge: bridge control structure +* @connector: the connector to read EDID for drivers/gpu/drm/drm_bridge.c int drm_bridge_get_modes(struct drm_bridge *bridge, · line 1223
Quoted 10 lines
drivers/gpu/drm/drm_bridge.c +10
-0+* +* 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_PTRinstead 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.
Quoted 29 lines
Quoted 17 lines
drivers/gpu/drm/drm_bridge.c +16
-0+*/ +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 ; +drivers/gpu/drm/drm_bridge.c int drm_bridge_get_modes(struct drm_bridge *bridge, · line 1249
Quoted 10 lines
drivers/gpu/drm/drm_bridge.c +10
-0+ 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]
Signature · 1 line
Jani Nikula, Intel
PATCH 2/2 drm/bridge: switch to drm_bridge_read_edid()
In reply to Jani NikulaPrefer using the struct drm_edid based functions.
--- drivers/gpu/drm/drm_bridge_connector.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/gpu/drm/drm_bridge_connector.c +8 -8
@@ -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 ; }
Signature · 1 line
2.39.2
Quoted 7 lines · 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_edidinfrastructure. 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.
Quoted 8 lines
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.
Quoted 19 lines
Thanks,
Jani.[1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
Jani Nikula (2):
drm/bridge: add ->edid_read hook anddrm_bridge_edid_read()drm/bridge: switch todrm_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(-)
Signature · 1 line
Jani Nikula, Intel
Quoted 15 lines · Jani Nikula
Quoted 14 lines · 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_edidinfrastructure. 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.
Quoted 3 lines
Quoted 1 line
Bridge maintainers, please instruct how to best proceed with this.
Ping.
Ping.
Quoted 26 lines
The two patches posted here could be merged, to add something to build the later commits on gradually.
BR,
Jani.Quoted 19 lines
Thanks,
Jani.[1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
Jani Nikula (2):
drm/bridge: add ->edid_read hook anddrm_bridge_edid_read()drm/bridge: switch todrm_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(-)
Signature · 1 line
Jani Nikula, Intel
Neil Armstrong
maintainerQuoted 16 lines · Jani Nikula
Quoted 15 lines · Jani Nikula
Quoted 14 lines · 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_edidinfrastructure. 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.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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
Quoted 28 lines
Quoted 27 lines
The two patches posted here could be merged, to add something to build the later commits on gradually.
BR,
Jani.Quoted 19 lines
Thanks,
Jani.[1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
Jani Nikula (2):
drm/bridge: add ->edid_read hook anddrm_bridge_edid_read()drm/bridge: switch todrm_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(-)
Quoted 19 lines · Neil Armstrong
Quoted 18 lines · Jani Nikula
Quoted 17 lines · Jani Nikula
Quoted 15 lines · 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_edidinfrastructure. 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.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Ping.
Ping.
Sorry for the delay, I would have preferred changing the
get_edidto return adrm_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().
Quoted 2 lines
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.
Quoted 32 lines
Neil
Quoted 28 lines
Quoted 27 lines
The two patches posted here could be merged, to add something to build the later commits on gradually.
BR,
Jani.Quoted 19 lines
Thanks,
Jani.[1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
Jani Nikula (2):
drm/bridge: add ->edid_read hook anddrm_bridge_edid_read()drm/bridge: switch todrm_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(-)
Signature · 1 line
Jani Nikula, Intel
Quoted 34 lines · Jani Nikula
Quoted 19 lines · Neil Armstrong
Quoted 18 lines · Jani Nikula
Quoted 17 lines · Jani Nikula
Quoted 15 lines · 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_edidinfrastructure. 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.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Ping.
Ping.
Sorry for the delay, I would have preferred changing the
get_edidto return adrm_edid, but I understand the task is too high, could you instead use @get_drm_edid instead of @edid_read ?
edid_readmatches the changes indrm_edid.c, going fromdrm_get_edid()todrm_edid_read().There's a nice symmetry when ->get_edid() hooks using
drm_get_edid()are converted to ->edid_read() hooks usingdrm_edid_read().
Quoted 6 lines
Quoted 2 lines
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
Signature · 1 line
Jani Nikula, Intel
Quoted 2 lines · Jani Nikula
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.
Quoted 6 lines
BR,
Jani.[1] https://gitlab.freedesktop.org/jani/linux/-/commits/drm-edid-bridge
Signature · 1 line
Jani Nikula, Intel