From: Ville Syrjälä ville.syrjala@linux.intel.com
CEA ext block revisions 1 and 2 do not contain the data block collection. Instead that section of the extension block is marked as reserved for 8 byte timing descriptors. Revision 3 changed it to contain the CEA data block collection instead.
Most places that iterate the data blocks already check for revision >= 3, but drm_detect_hdmi_monitor() and drm_detect_monitor_audio() do not. So in theory when encountering rev 1 or 2 CEA extension block they could end up misinterpreting whatever data is in the reserved section as CEA data blocks.
Let's have cea_db_offsets() do the revision check so that the callers don't even have worry about it.
Cc: Jean Delvare jdelvare@suse.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 82a4ceed3fcf..7b3072fc550b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) static int cea_db_offsets(const u8 *cea, int *start, int *end) { + if (cea_revision(cea) < 3) + return -ENOTSUPP; + /* DisplayID CTA extension blocks and top-level CEA EDID * block header definitions differ in the following bytes: * 1) Byte 2 of the header specifies length differently,
From: Ville Syrjälä ville.syrjala@linux.intel.com
Let's make cea_db_offsets() a bit more convenient to use by setting the start/end offsets to zero whenever the data block collection isn't present. This makes it safe for the caller to blindly iterate the data blocks even if there are none.
Cc: Jean Delvare jdelvare@suse.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7b3072fc550b..e5905dc764c1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) static int cea_db_offsets(const u8 *cea, int *start, int *end) { + *start = 0; + *end = 0; + if (cea_revision(cea) < 3) return -ENOTSUPP;
@@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) if (cea_revision(cea) >= 3) { int i, start, end;
- if (cea_db_offsets(cea, &start, &end)) { - start = 0; - end = 0; - } + cea_db_offsets(cea, &start, &end);
for_each_cea_db(cea, i, start, end) { db = &cea[i];
Hi Ville,
On Mon, 2 Sep 2019 16:15:46 +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Let's make cea_db_offsets() a bit more convenient to use by setting the start/end offsets to zero whenever the data block collection isn't present. This makes it safe for the caller to blindly iterate the data blocks even if there are none.
Cc: Jean Delvare jdelvare@suse.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_edid.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7b3072fc550b..e5905dc764c1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) static int cea_db_offsets(const u8 *cea, int *start, int *end) {
- *start = 0;
- *end = 0;
- if (cea_revision(cea) < 3) return -ENOTSUPP;
@@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) if (cea_revision(cea) >= 3) { int i, start, end;
if (cea_db_offsets(cea, &start, &end)) {start = 0;end = 0;}
cea_db_offsets(cea, &start, &end);for_each_cea_db(cea, i, start, end) { db = &cea[i];
Not sure if that's really needed. As it stands there's only one function which wants to continue after cea_db_offsets() fails, all others just bail out at that point. Now that cea_db_offsets() checks for revision >= 3, the construct above could simply become:
int i, start, end;
if (cea_db_offsets(cea, &start, &end) == 0) { for_each_cea_db(cea, i, start, end) { db = &cea[i];
which is IMHO more elegant and does not require zeroing start and end in cea_db_offsets().
On Tue, Sep 10, 2019 at 11:46:20AM +0200, Jean Delvare wrote:
Hi Ville,
On Mon, 2 Sep 2019 16:15:46 +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Let's make cea_db_offsets() a bit more convenient to use by setting the start/end offsets to zero whenever the data block collection isn't present. This makes it safe for the caller to blindly iterate the data blocks even if there are none.
Cc: Jean Delvare jdelvare@suse.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_edid.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7b3072fc550b..e5905dc764c1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) static int cea_db_offsets(const u8 *cea, int *start, int *end) {
- *start = 0;
- *end = 0;
- if (cea_revision(cea) < 3) return -ENOTSUPP;
@@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) if (cea_revision(cea) >= 3) { int i, start, end;
if (cea_db_offsets(cea, &start, &end)) {start = 0;end = 0;}
cea_db_offsets(cea, &start, &end);for_each_cea_db(cea, i, start, end) { db = &cea[i];
Not sure if that's really needed. As it stands there's only one function which wants to continue after cea_db_offsets() fails, all others just bail out at that point. Now that cea_db_offsets() checks for revision >= 3, the construct above could simply become:
int i, start, end;
if (cea_db_offsets(cea, &start, &end) == 0) { for_each_cea_db(cea, i, start, end) { db = &cea[i];
which is IMHO more elegant and does not require zeroing start and end in cea_db_offsets().
I dislike indentation. Also could perhaps even move the cea_db_offsets() into the for_each_cea_db() macro so that the caller doesn't have to care about these mundane details at all.
On Tue, 10 Sep 2019 12:48:42 +0300, Ville Syrjälä wrote:
On Tue, Sep 10, 2019 at 11:46:20AM +0200, Jean Delvare wrote:
Hi Ville,
On Mon, 2 Sep 2019 16:15:46 +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Let's make cea_db_offsets() a bit more convenient to use by setting the start/end offsets to zero whenever the data block collection isn't present. This makes it safe for the caller to blindly iterate the data blocks even if there are none.
Cc: Jean Delvare jdelvare@suse.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_edid.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7b3072fc550b..e5905dc764c1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) static int cea_db_offsets(const u8 *cea, int *start, int *end) {
- *start = 0;
- *end = 0;
- if (cea_revision(cea) < 3) return -ENOTSUPP;
@@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) if (cea_revision(cea) >= 3) { int i, start, end;
if (cea_db_offsets(cea, &start, &end)) {start = 0;end = 0;}
cea_db_offsets(cea, &start, &end);for_each_cea_db(cea, i, start, end) { db = &cea[i];
Not sure if that's really needed. As it stands there's only one function which wants to continue after cea_db_offsets() fails, all others just bail out at that point. Now that cea_db_offsets() checks for revision >= 3, the construct above could simply become:
int i, start, end;
if (cea_db_offsets(cea, &start, &end) == 0) { for_each_cea_db(cea, i, start, end) { db = &cea[i];
which is IMHO more elegant and does not require zeroing start and end in cea_db_offsets().
I dislike indentation.
It would be the same as currently, so it's not that bad. But well I'm not maintaining that piece of code so it's not my call anyway.
Also could perhaps even move the cea_db_offsets() into the for_each_cea_db() macro so that the caller doesn't have to care about these mundane details at all.
If the macro stays readable, why not.
Hi Ville,
On Mon, 2 Sep 2019 16:15:45 +0300, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
CEA ext block revisions 1 and 2 do not contain the data block collection. Instead that section of the extension block is marked as reserved for 8 byte timing descriptors. Revision 3 changed it to contain the CEA data block collection instead.
Most places that iterate the data blocks already check for revision >= 3, but drm_detect_hdmi_monitor() and drm_detect_monitor_audio() do not. So in theory when encountering rev 1 or 2 CEA extension block they could end up misinterpreting whatever data is in the reserved section as CEA data blocks.
Let's have cea_db_offsets() do the revision check so that the callers don't even have worry about it.
Cc: Jean Delvare jdelvare@suse.de Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_edid.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 82a4ceed3fcf..7b3072fc550b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) static int cea_db_offsets(const u8 *cea, int *start, int *end) {
- if (cea_revision(cea) < 3)
return -ENOTSUPP;- /* DisplayID CTA extension blocks and top-level CEA EDID
- block header definitions differ in the following bytes:
- Byte 2 of the header specifies length differently,
Reviewed-by: Jean Delvare jdelvare@suse.de
I like it, but then we need a subsequent patch to remove the now redundant checks in add_cea_modes(), drm_edid_to_eld(), drm_edid_to_sad() and drm_edid_to_speaker_allocation().
These last 2 functions are the ones my own patch modifies, so some care is needed. If cea_db_offsets() now returns an error when CEA revisions < 3, then these functions want to return 0 in that case (otherwise you effectively undo the change I proposed).
By the way, both functions issue a debug message "SAD: invalid data block offsets" when cea_db_offsets() returns an error, which becomes misleading after your change. I think we want to move this message inside cea_db_offsets() and only print it in the -ERANGE case.
dri-devel@lists.freedesktop.org
-
Jean Delvare -
Ville Syrjala -
Ville Syrjälä