This patchset fixes issues about EDID and EDID Extension block handling:
1. We currently don't return any EDID information if there are issues with reading extension blocks. There are devices however which announce to have EDID extension blocks although these are not readable. If reading extension blocks fails we should at least return the base block (with a corrected extension blcok flag). 2. There are broken legacy DDC devices around which wrongly announce to have ED extension blocks. Since the segment address is ignored on these devices we will get a base block returned for any uneven block number. Detect this situation and abort reading further blocks. 3. When we read EDID extension blocks but fail to validate them we discard them and correct the EDID block count in the base block. There may however be a block map in the first extesion block which we should also fix. Similarly, don't fail if we cannot allocate enough contiguous memory for all EDID extension blocks: rather than failing completely, it is better to return at least the base block and if possible all extension blocks for which there is memory. 4. EDID 'firmware'-files: * Treat the, the same way as EDIDs read from DDC channels: - use the same routines to validate and fix them. - feed them when reading * Support different EDID files for different connectors. 5. Consolidate EDID error handling: Store information about the error status of the last DDC read in the drm_connector structure. This data can be used: - in the driver to detect certain issues with DDC read out. - provide a finer control over repeated error logging - instead of disabling EDID error logging entirely if an EDID error has been detected, only disable it for those errors which have been seen also on the previous DDC read cycle. 6. Move EDID-related functions to drm_edid.h The handling of EDID related DRM functions seemed to be a bit arbitrary: although drm_edid.h had been created, new functions were often added to drm_crtc.h
Egbert Eich (17): DRM/KMS/EDID: Mask out Segment Bits when calculating Offset. DRM/KMS/EDID: 0x7e -> EDID_EXTENSION_FLAG_OFFSET DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails. DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block. DRM/KMS/EDID: Fix up EEDID Map Blocks if Extension block count has changed. DRM/exynos: Fix Memory Leak: free EDID blcok returned by drm_get_edid(). DRM/KMS/EDID: Move drm_edid_load.o to drm.ko DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID. DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID is read. DRM/KMS/EDID: Cache EDID blobs with extensions. DRM/KMS/EDID: Avoid failing on krealloc on EDID blocks. DRM/KMS/EDID: Consolidate EDID Error Handling. DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files. DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid(). DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid(). DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid(). DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.
drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/ast/ast_mode.c | 1 + drivers/gpu/drm/drm_crtc.c | 1 + drivers/gpu/drm/drm_crtc_helper.c | 6 +- drivers/gpu/drm/drm_edid.c | 303 ++++++++++++++++++++++++---- drivers/gpu/drm/drm_edid_load.c | 115 +++++------ drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + drivers/gpu/drm/gma500/cdv_intel_dp.c | 1 + drivers/gpu/drm/gma500/oaktrail_lvds.c | 1 + drivers/gpu/drm/gma500/psb_intel_modes.c | 1 + drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 13 +- include/drm/drm_edid.h | 24 ++- 14 files changed, 348 insertions(+), 124 deletions(-)
This patch is a bit cosmetic as the variable size will truncate the start address anyway but for readability it should be made explicite that the lowest bit in the EDID block number determines the I2C start address.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index fadcd44..049fa52 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -258,7 +258,7 @@ static int drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, int block, int len) { - unsigned char start = block * EDID_LENGTH; + unsigned char start = (block & 0x01) * EDID_LENGTH; unsigned char segment = block >> 1; unsigned char xfers = segment ? 3 : 2; int ret, retries = 5;
If I2C readout fails for an extension block but we have read a valid base block, don't fail completely but at least return the base block.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index f90d968..b258646 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -355,8 +355,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block + (valid_extensions + 1) * EDID_LENGTH, - j, EDID_LENGTH)) - goto out; + j, EDID_LENGTH)) { + valid_extensions = 0; + goto no_more; + } if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { valid_extensions++; break; @@ -368,6 +370,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) drm_get_connector_name(connector), j); }
+ no_more: if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
There are displays which announce EDID extension blocks in the Extension Flag of the EDID base block although they are not EDDC capable (ie. take a segment address at I2C slave address 0x30). We test this by looking for an EDID header which is only possible in the base block. If the segment address is not taken into account, this block will be identical to the base block in which case we stop reading further EEDID blocks, correct the extension flag and just return the base block.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index b258646..ec6f3bb 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -361,6 +361,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) } if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { valid_extensions++; + /* Test if base block announced extension blocks although + * display is not EDDC capable. + */ + if (j == 2) { + int k; + for (k = 0; k < sizeof(edid_header); k++) + if (block[(EDID_LENGTH * 2) + k] != edid_header[k]) + break; + if (k == sizeof(edid_header)) { + valid_extensions = 0; + goto no_more; + } + } break; } }
EEDID v1.3 mandates map blocks if more than one EDID extension block is used while in v1.4 they are optional. If the extension count has been changed (because some extension blocks were not readable) those map blocks need fixing. In case of v1.4 or higher we simply eliminate all map blocks as this is less time consuming. For v1.3 we scrap any exsisting map blocks and recreate them from scratch.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid.c | 105 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index ec6f3bb..d1b9d67 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -44,7 +44,11 @@ #define EDID_DETAILED_TIMINGS 4
#define EDID_EXTENSION_FLAG_OFFSET ((size_t)&((struct edid *)0)->extensions) -#define EDID_CHECKSUM_OFFSET ((size_t)&((struct edid *)0)->checksum) +#define EDID_CHECKSUM_OFFSET ((size_t)&((struct edid *)0)->checksum) +#define EDID_VERSION_MAJOR_OFFSET ((size_t)&((struct edid *)0)->version) +#define EDID_VERSION_MINOR_OFFSET ((size_t)&((struct edid *)0)->revision) +#define EEDID_BLOCK_MAP_FLAG 0xF0 + /* * EDID blocks out in the wild have a variety of bugs, try to collect * them here (note that userspace may work around broken monitors first, @@ -318,6 +322,92 @@ static bool drm_edid_is_zero(u8 *in_edid, int length) return true; }
+static void +fix_map(u8 *block, int cnt) +{ + int i; + u8 csum; + + if (--cnt > 127) + cnt = 127; + memset(block, 0, 128); + block[0] = EEDID_BLOCK_MAP_FLAG; + csum = block[0]; + + for (i = 1; i < cnt; i++) { + block[i] = block[i * EDID_LENGTH]; + csum += block[i]; + } + block[127] = (u8)(0x100 - csum); +} + +static int +fixup_blockmaps(u8 **blockp, int eblock_cnt) +{ + int i; + u8 *block = *blockp; + + if (block[EDID_VERSION_MAJOR_OFFSET] > 1) + return 0; + if (block[EDID_VERSION_MINOR_OFFSET] < 3) + return 0; + if (eblock_cnt == 1) { + if (block[EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) + return 0; + else + return 1; + } + if (block[EDID_VERSION_MINOR_OFFSET] >= 4) { + /* blockmaps are optional: simply toss them */ + for (i = 1; i <= eblock_cnt; i++) { + if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) { + if (i < eblock_cnt) + memmove(&block[i * EDID_LENGTH], + &block[(i + 1) * EDID_LENGTH], + (eblock_cnt-i) * EDID_LENGTH); + i--; + eblock_cnt--; + } + } + } else { + int total_cnt = block[EDID_EXTENSION_FLAG_OFFSET]; + for (i = 1; i <= eblock_cnt; i++) { + if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) { + if (i == 1 || i == 128) /* correct map block locations */ + continue; + if (i < eblock_cnt) + memmove(&block[i * EDID_LENGTH], + &block[(i + 1) * EDID_LENGTH], + (eblock_cnt-i) * EDID_LENGTH); + i--; + eblock_cnt--; + continue; + } else if (i == 1 || i == 128) { + if (eblock_cnt >= total_cnt) { + u8 *tmp_p; + tmp_p = krealloc(block, (eblock_cnt + 2) * EDID_LENGTH, GFP_KERNEL); + if (!tmp_p) + return -ENOMEM; + *blockp = block = tmp_p; + total_cnt = eblock_cnt + 1; + } + eblock_cnt++; + memmove(&block[(i + 1) * EDID_LENGTH], + &block[i * EDID_LENGTH], + (eblock_cnt-i) * EDID_LENGTH); + } + } + fix_map(&block[EDID_LENGTH], eblock_cnt); + + if (eblock_cnt == 129) + return 128; + + if (eblock_cnt > 129) + fix_map(&block[EDID_LENGTH * 128], eblock_cnt - 127); + } + return eblock_cnt; +} + static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -385,10 +475,15 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
no_more: if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { - block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; - block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; - new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); - if (!new) + if (valid_extensions) + valid_extensions = fixup_blockmaps(&block, valid_extensions); + if (valid_extensions >= 0) { + block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; + block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; + new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); + if (!new) + goto out; + } else goto out; block = new; }
At Mon, 19 Nov 2012 15:23:06 -0500, "Egbert Eich " <"eich@novell.com> wrote:
EEDID v1.3 mandates map blocks if more than one EDID extension block is used while in v1.4 they are optional. If the extension count has been changed (because some extension blocks were not readable) those map blocks need fixing. In case of v1.4 or higher we simply eliminate all map blocks as this is less time consuming. For v1.3 we scrap any exsisting map blocks and recreate them from scratch.
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/drm_edid.c | 105 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index ec6f3bb..d1b9d67 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -44,7 +44,11 @@ #define EDID_DETAILED_TIMINGS 4
#define EDID_EXTENSION_FLAG_OFFSET ((size_t)&((struct edid *)0)->extensions) -#define EDID_CHECKSUM_OFFSET ((size_t)&((struct edid *)0)->checksum) +#define EDID_CHECKSUM_OFFSET ((size_t)&((struct edid *)0)->checksum) +#define EDID_VERSION_MAJOR_OFFSET ((size_t)&((struct edid *)0)->version) +#define EDID_VERSION_MINOR_OFFSET ((size_t)&((struct edid *)0)->revision)
Better to use offsetof().
Takashi
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm); + kfree(raw_edid); } else { return -ENODEV; }
Could you please re-send this patch updating like below?
For example,
[PATCH ] drm/exynos: fix memory leak to edid
some comments.
Thanks, Inki Dae
2012/11/20 Egbert Eich eich@suse.de
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm);
kfree(raw_edid); } else { return -ENODEV; }-- 1.7.7
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
drm_get_edid() returns a pointer to an EDID block. The caller is responsible to free this pointer itself. Here the pointer gets assigned to the local variable raw_edid. Therefore it should be freed before the variable goes out of scope.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm); + kfree(raw_edid); } else { return -ENODEV; }
On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich eich@suse.de wrote:
drm_get_edid() returns a pointer to an EDID block. The caller is responsible to free this pointer itself. Here the pointer gets assigned to the local variable raw_edid. Therefore it should be freed before the variable goes out of scope.
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm);
kfree(raw_edid);
This will actually cause the memory to be freed twice.
The reason this happens is drm_get_edid attaches this to connector->display_info.raw_edid, which is then freed in the exynos_drm_connector function that gets the edid.
The whole thing is ugly, and needs to be revised. I've uploaded a patch to refactor this against the chromium tree, but haven't yet rebased against upstream. See https://gerrit.chromium.org/gerrit/#/c/38406/
For now, please drop this patch.
Sean
} else { return -ENODEV; }-- 1.7.7
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 20 Nov 2012, Sean Paul seanpaul@chromium.org wrote:
On Tue, Nov 20, 2012 at 4:30 AM, Egbert Eich eich@suse.de wrote:
drm_get_edid() returns a pointer to an EDID block. The caller is responsible to free this pointer itself. Here the pointer gets assigned to the local variable raw_edid. Therefore it should be freed before the variable goes out of scope.
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm);
kfree(raw_edid);This will actually cause the memory to be freed twice.
The reason this happens is drm_get_edid attaches this to connector->display_info.raw_edid, which is then freed in the exynos_drm_connector function that gets the edid.
The whole thing is ugly, and needs to be revised. I've uploaded a patch to refactor this against the chromium tree, but haven't yet rebased against upstream. See https://gerrit.chromium.org/gerrit/#/c/38406/
The patch is good. connector->display_info.raw_edid is gone since
commit 451023dc32d4542c21b52ad1692e6e01cb75b099 Author: Jani Nikula jani.nikula@intel.com Date: Wed Aug 15 09:32:39 2012 +0000
drm: remove the raw_edid field from struct drm_display_info
BR, Jani.
For now, please drop this patch.
Sean
} else { return -ENODEV; }-- 1.7.7
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Ok, I modifed the subject of your patch like below and applied.
drm/exynos: fix memory lean to EDID block
Thanks, Inki Dae
2012/11/20 Egbert Eich eich@suse.de
drm_get_edid() returns a pointer to an EDID block. The caller is responsible to free this pointer itself. Here the pointer gets assigned to the local variable raw_edid. Therefore it should be freed before the variable goes out of scope.
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bc87bca 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1293,6 +1293,7 @@ static int hdmi_get_edid(void *ctx, struct drm_connector *connector, DRM_DEBUG_KMS("%s : width[%d] x height[%d]\n", (hdata->dvi_mode ? "dvi monitor" : "hdmi monitor"), raw_edid->width_cm, raw_edid->height_cm);
kfree(raw_edid); } else { return -ENODEV; }-- 1.7.7
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
EDIDs are an integral concept of connectors, connectors are a concept of drm core also drm_edid.o is already part of this drm core. Overridden, 'firmware-supplied' EDIDs should be treated exactly the same as device supplied ones. Therefore move drm_edid_load from the helper level to the drm core.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/Makefile | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 2ff5cef..76ca269 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -16,11 +16,11 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm-usb-y := drm_usb.o
drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
At Mon, 19 Nov 2012 15:23:08 -0500, "Egbert Eich " <"eich@novell.com> wrote:
EDIDs are an integral concept of connectors, connectors are a concept of drm core also drm_edid.o is already part of this drm core. Overridden, 'firmware-supplied' EDIDs should be treated exactly the same as device supplied ones. Therefore move drm_edid_load from the helper level to the drm core.
Signed-off-by: Egbert Eich eich@suse.de
You need to fix Kconfig as well. And I think the dependency on CONFIG_FW_LOADER is missing there, too. It should have "select FW_LOADER".
Takashi
drivers/gpu/drm/Makefile | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 2ff5cef..76ca269 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -16,11 +16,11 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm-usb-y := drm_usb.o
drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
1.7.7
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid.c | 77 ++++++++++++++++++++++++++++++++------- drivers/gpu/drm/drm_edid_load.c | 54 ++++++--------------------- include/drm/drm_edid.h | 1 + 3 files changed, 77 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index d1b9d67..7bdae6e 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -408,6 +408,29 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt) return eblock_cnt; }
+static int +fixup_edid(u8 **blockp, int valid_extensions) +{ + u8 *new = NULL; + + if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) { + + if (valid_extensions) + valid_extensions = fixup_blockmaps(blockp, valid_extensions); + + if (valid_extensions >= 0) { + (*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; + (*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; + new = krealloc(*blockp, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); + } + if (!new) + kfree(*blockp); + + *blockp = new; + } + return (new ? valid_extensions : -ENOMEM); +} + static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -474,19 +497,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) }
no_more: - if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { - if (valid_extensions) - valid_extensions = fixup_blockmaps(&block, valid_extensions); - if (valid_extensions >= 0) { - block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; - block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; - new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); - if (!new) - goto out; - } else - goto out; - block = new; - } + fixup_edid(&block, valid_extensions);
return block;
@@ -503,6 +514,46 @@ out: }
/** + * Validate an entire EDID blob. + * \param connector: drm_connector struct of the used connector. + * \param blockp: pointer to address of an raw EDID data block. + * \param len: size if block in bytes. + * + * validate block and return corrected block in \param block. + * \return: number of valid extensions or -errno if unsuccessful. + */ +int +drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) +{ + int n_blocks = len / EDID_LENGTH; + int valid_extensions = 0, ret = 0; + bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + + if (!blockp || !*blockp) + ret = -EINVAL; + else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) { + kfree(*blockp); + *blockp = NULL; + ret = -EINVAL; + } + if (!ret) { + n_blocks--; + if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks) + n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET]; + + while (n_blocks--) { + if (drm_edid_block_valid(*blockp + (valid_extensions + 1) * EDID_LENGTH, + valid_extensions + 1, print_bad_edid)) + valid_extensions++; + } + ret = fixup_edid(blockp, valid_extensions); + } + if (ret < 0) + connector->bad_edid_counter++; + return ret; +} + +/** * Probe DDC presence. * * \param adapter : i2c device adaptor diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 38d3943..6541c1f 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -119,11 +119,10 @@ static u8 *edid_load(struct drm_connector *connector, char *name, { const struct firmware *fw; struct platform_device *pdev; - u8 *fwdata = NULL, *edid, *new_edid; + u8 *fwdata = NULL; + struct edid *edid; int fwsize, expected; int builtin = 0, err = 0; - int i, valid_extensions = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
pdev = platform_device_register_simple(connector_name, -1, NULL, 0); if (IS_ERR(pdev)) { @@ -137,7 +136,7 @@ static u8 *edid_load(struct drm_connector *connector, char *name, platform_device_unregister(pdev);
if (err) { - i = 0; + int i = 0; while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i])) i++; if (i < GENERIC_EDIDS) { @@ -174,49 +173,20 @@ static u8 *edid_load(struct drm_connector *connector, char *name, } memcpy(edid, fwdata, fwsize);
- if (!drm_edid_block_valid(edid, 0, print_bad_edid)) { - connector->bad_edid_counter++; - DRM_ERROR("Base block of EDID firmware "%s" is invalid ", - name); - kfree(edid); - err = -EINVAL; - goto relfw_out; - } - - for (i = 1; i <= edid[0x7e]; i++) { - if (i != valid_extensions + 1) - memcpy(edid + (valid_extensions + 1) * EDID_LENGTH, - edid + i * EDID_LENGTH, EDID_LENGTH); - if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid)) - valid_extensions++; - } - - if (valid_extensions != edid[0x7e]) { - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; - DRM_INFO("Found %d valid extensions instead of %d in EDID data " - ""%s" for connector "%s"\n", valid_extensions, - edid[0x7e], name, connector_name); - edid[0x7e] = valid_extensions; - new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, - GFP_KERNEL); - if (new_edid == NULL) { - err = -ENOMEM; - kfree(edid); - goto relfw_out; - } - edid = new_edid; - } - - DRM_INFO("Got %s EDID base block and %d extension%s from " - ""%s" for connector "%s"\n", builtin ? "built-in" : - "external", valid_extensions, valid_extensions == 1 ? "" : "s", - name, connector_name); + err = drm_validate_edid_blob(connector, (u8 **)&edid, fwsize); + if (err < 0) + DRM_ERROR("EDID firmware "%s" is invalid ", name); + else + DRM_INFO("Got %s EDID base block and %d extension%s from " + ""%s" for connector "%s"\n", builtin ? "built-in" : + "external", edid->extensions, edid->extensions == 1 ? "" : "s", + name, connector_name);
relfw_out: release_firmware(fw);
out: - if (err) + if (err < 0) return ERR_PTR(err);
return edid; diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 0cac551..3e8ef06 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -253,5 +253,6 @@ int drm_av_sync_delay(struct drm_connector *connector, struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct drm_display_mode *mode); int drm_load_edid_firmware(struct drm_connector *connector); +int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
#endif /* __DRM_EDID_H__ */
At Mon, 19 Nov 2012 15:23:09 -0500, "Egbert Eich " <"eich@novell.com> wrote:
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/drm_edid.c | 77 ++++++++++++++++++++++++++++++++------- drivers/gpu/drm/drm_edid_load.c | 54 ++++++--------------------- include/drm/drm_edid.h | 1 + 3 files changed, 77 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index d1b9d67..7bdae6e 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -408,6 +408,29 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt) return eblock_cnt; }
+static int +fixup_edid(u8 **blockp, int valid_extensions) +{
- u8 *new = NULL;
- if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) {
if (valid_extensions)valid_extensions = fixup_blockmaps(blockp, valid_extensions);if (valid_extensions >= 0) {(*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;(*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;new = krealloc(*blockp, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);}if (!new)kfree(*blockp);*blockp = new;- }
- return (new ? valid_extensions : -ENOMEM);
So this function returns -ENOMEM if valid_extensions is equal with block[EDID_EXTENSION_FLAG_OFFSET]...?
Takashi
+}
static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -474,19 +497,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) }
no_more:
- if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) {
if (valid_extensions)valid_extensions = fixup_blockmaps(&block, valid_extensions);if (valid_extensions >= 0) {block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions;block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);if (!new)goto out;} elsegoto out;block = new;- }
fixup_edid(&block, valid_extensions);
return block;
@@ -503,6 +514,46 @@ out: }
/**
- Validate an entire EDID blob.
- \param connector: drm_connector struct of the used connector.
- \param blockp: pointer to address of an raw EDID data block.
- \param len: size if block in bytes.
- validate block and return corrected block in \param block.
- \return: number of valid extensions or -errno if unsuccessful.
- */
+int +drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) +{
- int n_blocks = len / EDID_LENGTH;
- int valid_extensions = 0, ret = 0;
- bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
- if (!blockp || !*blockp)
ret = -EINVAL;- else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) {
kfree(*blockp);*blockp = NULL;ret = -EINVAL;- }
- if (!ret) {
n_blocks--;if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks)n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];while (n_blocks--) {if (drm_edid_block_valid(*blockp + (valid_extensions + 1) * EDID_LENGTH,valid_extensions + 1, print_bad_edid))valid_extensions++;}ret = fixup_edid(blockp, valid_extensions);- }
- if (ret < 0)
connector->bad_edid_counter++;- return ret;
+}
+/**
- Probe DDC presence.
- \param adapter : i2c device adaptor
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 38d3943..6541c1f 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -119,11 +119,10 @@ static u8 *edid_load(struct drm_connector *connector, char *name, { const struct firmware *fw; struct platform_device *pdev;
- u8 *fwdata = NULL, *edid, *new_edid;
- u8 *fwdata = NULL;
- struct edid *edid; int fwsize, expected; int builtin = 0, err = 0;
int i, valid_extensions = 0;
bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
pdev = platform_device_register_simple(connector_name, -1, NULL, 0); if (IS_ERR(pdev)) {
@@ -137,7 +136,7 @@ static u8 *edid_load(struct drm_connector *connector, char *name, platform_device_unregister(pdev);
if (err) {
i = 0;
int i = 0;@@ -174,49 +173,20 @@ static u8 *edid_load(struct drm_connector *connector, char *name, } memcpy(edid, fwdata, fwsize);
- if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
connector->bad_edid_counter++;DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",name);kfree(edid);err = -EINVAL;goto relfw_out;- }
- for (i = 1; i <= edid[0x7e]; i++) {
if (i != valid_extensions + 1)memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,edid + i * EDID_LENGTH, EDID_LENGTH);if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))valid_extensions++;- }
- if (valid_extensions != edid[0x7e]) {
edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;DRM_INFO("Found %d valid extensions instead of %d in EDID data ""\"%s\" for connector \"%s\"\n", valid_extensions,edid[0x7e], name, connector_name);edid[0x7e] = valid_extensions;new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,GFP_KERNEL);if (new_edid == NULL) {err = -ENOMEM;kfree(edid);goto relfw_out;}edid = new_edid;- }
- DRM_INFO("Got %s EDID base block and %d extension%s from "
"\"%s\" for connector \"%s\"\n", builtin ? "built-in" :"external", valid_extensions, valid_extensions == 1 ? "" : "s",name, connector_name);
- err = drm_validate_edid_blob(connector, (u8 **)&edid, fwsize);
- if (err < 0)
DRM_ERROR("EDID firmware \"%s\" is invalid ", name);- else
DRM_INFO("Got %s EDID base block and %d extension%s from ""\"%s\" for connector \"%s\"\n", builtin ? "built-in" :"external", edid->extensions, edid->extensions == 1 ? "" : "s",name, connector_name);relfw_out: release_firmware(fw);
out:
- if (err)
if (err < 0) return ERR_PTR(err);
return edid;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 0cac551..3e8ef06 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -253,5 +253,6 @@ int drm_av_sync_delay(struct drm_connector *connector, struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct drm_display_mode *mode); int drm_load_edid_firmware(struct drm_connector *connector); +int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
#endif /* __DRM_EDID_H__ */
1.7.7
Firmware supplied EDIDs where fed in in drm_helper_probe_single_connector_modes() in place of calling the driver supplied get_modes() function. This has two problems: 1. Drivers don't call drm_get_edid() only from within get_modes(). 2. The get_modes() replacement in drm_load_edid_firmware() provided only the least common denominator of what driver provided get_modes() callbacks do. This patch now supplies a user provided 'firmware' EDID whenever drm_get_edid() is called if one is available. The user supplied EDID is thus used the same way as a display provided one (except for detecting the presence of a device thru EDID). Also it does so not on the helper level any more, the possibility for EDID loading now happens on the DRM KMS core level.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_crtc_helper.c | 6 +----- drivers/gpu/drm/drm_edid.c | 7 +++++++ drivers/gpu/drm/drm_edid_load.c | 22 +++++++++------------- include/drm/drm_edid.h | 4 +++- 4 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 1227adf..bc99595 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -119,11 +119,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, goto prune; }
-#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE - count = drm_load_edid_firmware(connector); - if (count == 0) -#endif - count = (*connector_funcs->get_modes)(connector); + count = (*connector_funcs->get_modes)(connector);
if (count == 0 && connector->status == connector_status_connected) count = drm_add_modes_noedid(connector, 1024, 768); diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7bdae6e..3e7df61 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -438,6 +438,13 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) u8 *block, *new; bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
+#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE + /* check if the user has specified a 'firmware' EDID file */ + block = (u8*)drm_load_edid_firmware(connector); + if (block) + return block; +#endif + if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) return NULL;
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 6541c1f..1475c6f 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -114,7 +114,7 @@ static u8 generic_edid[GENERIC_EDIDS][128] = { }, };
-static u8 *edid_load(struct drm_connector *connector, char *name, +static struct edid *edid_load(struct drm_connector *connector, char *name, char *connector_name) { const struct firmware *fw; @@ -192,36 +192,32 @@ out: return edid; }
-int drm_load_edid_firmware(struct drm_connector *connector) +struct edid * +drm_load_edid_firmware(struct drm_connector *connector) { char *connector_name = drm_get_connector_name(connector); char *edidname = edid_firmware, *last, *colon; - int ret; struct edid *edid;
if (*edidname == '\0') - return 0; + return NULL;
colon = strchr(edidname, ':'); if (colon != NULL) { if (strncmp(connector_name, edidname, colon - edidname)) - return 0; + return NULL; edidname = colon + 1; if (*edidname == '\0') - return 0; + return NULL; }
last = edidname + strlen(edidname) - 1; if (*last == '\n') *last = '\0';
- edid = (struct edid *) edid_load(connector, edidname, connector_name); + edid = edid_load(connector, edidname, connector_name); if (IS_ERR_OR_NULL(edid)) - return 0; + return NULL;
- drm_mode_connector_update_edid_property(connector, edid); - ret = drm_add_edid_modes(connector, edid); - kfree(edid); - - return ret; + return edid; } diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 3e8ef06..53c619c 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -252,7 +252,9 @@ int drm_av_sync_delay(struct drm_connector *connector, struct drm_display_mode *mode); struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct drm_display_mode *mode); -int drm_load_edid_firmware(struct drm_connector *connector); +#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE +struct edid *drm_load_edid_firmware(struct drm_connector *connector); +#endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
#endif /* __DRM_EDID_H__ */
According the the VESA specs there can be up to 254 EEDID extension blocks. Since we may read the EDID (including extensions) in 10 second intervals to probe for display hotplugging (at least in cases where no hardware hotplug detection exists) and I2C transfer is rather slow we may end up consuming a considerable amount on CPU time for just that. This patch caches the EDID block if it contains at least one extension. To determine if the blocks match we only tranfer the base block, on a match we use the cached data.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_crtc.c | 1 + drivers/gpu/drm/drm_edid.c | 43 ++++++++++++++++++++++++++++++++++++++++++- include/drm/drm_crtc.h | 1 + include/drm/drm_edid.h | 1 + 4 files changed, 45 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3533609..e283355 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector) drm_mode_remove(connector, mode);
mutex_lock(&dev->mode_config.mutex); + drm_cache_edid(connector, NULL); drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--; diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 3e7df61..410a54d 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -431,6 +431,41 @@ fixup_edid(u8 **blockp, int valid_extensions) return (new ? valid_extensions : -ENOMEM); }
+static bool +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp) +{ + if (connector->edid_cache && + connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] && + connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] && + connector->edid_cache->serial == (*edidp)->serial && + connector->edid_cache->input == (*edidp)->input) { + int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH; + struct edid *new = kmalloc(size, GFP_KERNEL); + if (!new) + return false; + DRM_DEBUG_KMS("Got EDID for %s from cache.\n",drm_get_connector_name(connector)); + memcpy(new, connector->edid_cache, size); + kfree(*edidp); + *edidp = new; + return true; + } + return false; +} + +void +drm_cache_edid(struct drm_connector *connector, struct edid *edid) +{ + struct edid *new = NULL; + kfree(connector->edid_cache); + if (edid) { + int size = (edid->extensions + 1) * EDID_LENGTH; + new = kmalloc(size, GFP_KERNEL); + if (new) + memcpy(new, edid, size); + } + connector->edid_cache = new; +} + static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -462,10 +497,14 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) if (i == 4) goto carp;
- /* if there's no extensions, we're done */ + /* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) return block;
+ /* see if EDID is in the cache - no need to read all extension blocks */ + if (compare_get_edid_from_cache(connector, (struct edid **)&block)) + return block; + new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; @@ -505,6 +544,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
no_more: fixup_edid(&block, valid_extensions); + drm_cache_edid(connector, (struct edid *)block);
return block;
@@ -513,6 +553,7 @@ carp: dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } + drm_cache_edid(connector, NULL); connector->bad_edid_counter++;
out: diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 49dd8c2..6a1054c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -601,6 +601,7 @@ struct drm_connector { struct drm_encoder *encoder; /* currently active encoder */
/* EDID bits */ + struct edid *edid_cache; uint8_t eld[MAX_ELD_BYTES]; bool dvi_dual; int max_tmds_clock; /* in MHz */ diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 53c619c..c880510 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); +void drm_cache_edid(struct drm_connector *connector, struct edid *edid);
#endif /* __DRM_EDID_H__ */
At Mon, 19 Nov 2012 15:23:11 -0500, "Egbert Eich " <"eich@novell.com> wrote:
According the the VESA specs there can be up to 254 EEDID extension blocks. Since we may read the EDID (including extensions) in 10 second intervals to probe for display hotplugging (at least in cases where no hardware hotplug detection exists) and I2C transfer is rather slow we may end up consuming a considerable amount on CPU time for just that. This patch caches the EDID block if it contains at least one extension. To determine if the blocks match we only tranfer the base block, on a match we use the cached data.
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/drm_crtc.c | 1 + drivers/gpu/drm/drm_edid.c | 43 ++++++++++++++++++++++++++++++++++++++++++- include/drm/drm_crtc.h | 1 + include/drm/drm_edid.h | 1 + 4 files changed, 45 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3533609..e283355 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector) drm_mode_remove(connector, mode);
mutex_lock(&dev->mode_config.mutex);
- drm_cache_edid(connector, NULL); drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 3e7df61..410a54d 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -431,6 +431,41 @@ fixup_edid(u8 **blockp, int valid_extensions) return (new ? valid_extensions : -ENOMEM); }
+static bool +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp) +{
- if (connector->edid_cache &&
connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&connector->edid_cache->serial == (*edidp)->serial &&connector->edid_cache->input == (*edidp)->input) {int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;struct edid *new = kmalloc(size, GFP_KERNEL);if (!new)return false;DRM_DEBUG_KMS("Got EDID for %s from cache.\n",drm_get_connector_name(connector));memcpy(new, connector->edid_cache, size);kfree(*edidp);
You can use kmemdup().
*edidp = new;return true;- }
- return false;
+}
+void +drm_cache_edid(struct drm_connector *connector, struct edid *edid) +{
- struct edid *new = NULL;
- kfree(connector->edid_cache);
- if (edid) {
int size = (edid->extensions + 1) * EDID_LENGTH;new = kmalloc(size, GFP_KERNEL);if (new)memcpy(new, edid, size);
Ditto.
Takashi
- }
- connector->edid_cache = new;
+}
static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -462,10 +497,14 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) if (i == 4) goto carp;
- /* if there's no extensions, we're done */
/* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) return block;
/* see if EDID is in the cache - no need to read all extension blocks */
if (compare_get_edid_from_cache(connector, (struct edid **)&block))
return block;new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out;
@@ -505,6 +544,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
no_more: fixup_edid(&block, valid_extensions);
drm_cache_edid(connector, (struct edid *)block);
return block;
@@ -513,6 +553,7 @@ carp: dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); }
- drm_cache_edid(connector, NULL); connector->bad_edid_counter++;
out: diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 49dd8c2..6a1054c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -601,6 +601,7 @@ struct drm_connector { struct drm_encoder *encoder; /* currently active encoder */
/* EDID bits */
- struct edid *edid_cache; uint8_t eld[MAX_ELD_BYTES]; bool dvi_dual; int max_tmds_clock; /* in MHz */
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 53c619c..c880510 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); +void drm_cache_edid(struct drm_connector *connector, struct edid *edid);
#endif /* __DRM_EDID_H__ */
1.7.7
There is no point to call krealloc() to shrink the amount of memory for EDID blocks: the implementation of krealloc() will ignore the size change and return the same chunk of memory anyway. So far we failed reading EDIDs when krealloc() failed. This may happen if the size required to store an EDID is larger than one page and the kernel is not able to find consecutive pages to allocate. In this case it is still better to return what we already have (in most cases the base block) and correct the extension block flag and the checksum accordingly than to fail completely.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid.c | 34 +++++++++++++--------------------- 1 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 410a54d..16c06d6 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -387,7 +387,7 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt) u8 *tmp_p; tmp_p = krealloc(block, (eblock_cnt + 2) * EDID_LENGTH, GFP_KERNEL); if (!tmp_p) - return -ENOMEM; + return eblock_cnt; *blockp = block = tmp_p; total_cnt = eblock_cnt + 1; } @@ -408,27 +408,16 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt) return eblock_cnt; }
-static int +static void fixup_edid(u8 **blockp, int valid_extensions) { - u8 *new = NULL; - - if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) { - + if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) { if (valid_extensions) valid_extensions = fixup_blockmaps(blockp, valid_extensions);
- if (valid_extensions >= 0) { - (*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; - (*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; - new = krealloc(*blockp, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); - } - if (!new) - kfree(*blockp); - - *blockp = new; + (*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; + (*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; } - return (new ? valid_extensions : -ENOMEM); }
static bool @@ -506,8 +495,12 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) return block;
new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); - if (!new) - goto out; + if (!new) { + dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: trunkating.\n", + drm_get_connector_name(connector), block[EDID_EXTENSION_FLAG_OFFSET] + 1); + valid_extensions = 0; + goto no_more; + } block = new;
for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) { @@ -594,9 +587,8 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) valid_extensions + 1, print_bad_edid)) valid_extensions++; } - ret = fixup_edid(blockp, valid_extensions); - } - if (ret < 0) + fixup_edid(blockp, valid_extensions); + } else connector->bad_edid_counter++; return ret; }
Consolidate the null_edid_counter and the bad_edid_counter into EDID error state flags which for the last EDID read are accessible from user. Errors are looged it the same error has not been present in the previous read of the EDID. This will reset the EDID error status for example when the monitor is changed but still prevents permanent EDID errors from piling up the the kernel logs.
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid.c | 101 +++++++++++++++++----------- drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 4 +- include/drm/drm_edid.h | 9 +++ 4 files changed, 72 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 16c06d6..6144150 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -158,6 +158,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid) } EXPORT_SYMBOL(drm_edid_header_is_valid);
+static bool drm_edid_is_zero(u8 *in_edid, int length) +{ + int i; + u32 *raw_edid = (u32 *)in_edid; + + for (i = 0; i < length / 4; i++) + if (*(raw_edid + i) != 0) + return false; + return true; +} + static int edid_fixup __read_mostly = 6; module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, @@ -167,11 +178,13 @@ MODULE_PARM_DESC(edid_fixup, * Sanity check the EDID block (base or extension). Return 0 if the block * doesn't check out, or 1 if it's valid. */ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) +unsigned +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags) { int i; u8 csum = 0; struct edid *edid = (struct edid *)raw_edid; + unsigned result = 0;
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6; @@ -183,27 +196,34 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); memcpy(raw_edid, edid_header, sizeof(edid_header)); } else { - goto bad; + result |= EDID_ERR_NO_BLOCK0; + if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) { + result |= EDID_ERR_NULL; + goto bad; + } } }
for (i = 0; i < EDID_LENGTH; i++) csum += raw_edid[i]; if (csum) { - if (print_bad_edid) { + if ((last_error_flags & EDID_ERR_CSUM) == 0) { DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum); }
/* allow CEA to slide through, switches mangle this */ if (raw_edid[0] != 0x02) - goto bad; + result |= EDID_ERR_CSUM; } + if (result) + goto bad;
/* per-block-type checks */ switch (raw_edid[0]) { case 0: /* base */ if (edid->version != 1) { DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version); + result |= EDID_ERR_UNSUPPORTED_VERSION; goto bad; }
@@ -215,15 +235,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1; + return 0;
bad: - if (raw_edid && print_bad_edid) { + if (raw_edid && last_error_flags != result) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); } - return 0; + return result; +} + +bool +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags) +{ + if (!drm_edid_block_check_error(raw_edid, block, last_error_flags)) + return true; + return false; } EXPORT_SYMBOL(drm_edid_block_valid);
@@ -242,7 +270,7 @@ bool drm_edid_is_valid(struct edid *edid) return false;
for (i = 0; i <= edid->extensions; i++) - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true)) + if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true)) return false;
return true; @@ -311,17 +339,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, return ret == xfers ? 0 : -1; }
-static bool drm_edid_is_zero(u8 *in_edid, int length) -{ - int i; - u32 *raw_edid = (u32 *)in_edid; - - for (i = 0; i < length / 4; i++) - if (*(raw_edid + i) != 0) - return false; - return true; -} - static void fix_map(u8 *block, int cnt) { @@ -460,7 +477,8 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { int i, j = 0, valid_extensions = 0; u8 *block, *new; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = 0;
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ @@ -476,12 +494,11 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(block, 0, print_bad_edid)) + result = drm_edid_block_check_error(block, 0, last_error_flags); + if (!result) break; - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { - connector->null_edid_counter++; + if (i == 0 && result & EDID_ERR_NULL) goto carp; - } } if (i == 4) goto carp; @@ -511,7 +528,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) valid_extensions = 0; goto no_more; } - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { + if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) { valid_extensions++; /* Test if base block announced extension blocks although * display is not EDDC capable. @@ -538,16 +555,17 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) no_more: fixup_edid(&block, valid_extensions); drm_cache_edid(connector, (struct edid *)block); + connector->last_edid_error_flags = 0;
return block;
carp: - if (print_bad_edid) { + if (last_error_flags != result) { dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } drm_cache_edid(connector, NULL); - connector->bad_edid_counter++; + connector->last_edid_error_flags = result;
out: kfree(block); @@ -567,29 +585,32 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) { int n_blocks = len / EDID_LENGTH; - int valid_extensions = 0, ret = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int valid_extensions = 0, ret = -EINVAL; + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
- if (!blockp || !*blockp) - ret = -EINVAL; - else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) { - kfree(*blockp); - *blockp = NULL; - ret = -EINVAL; + if (blockp && *blockp) { + if (n_blocks) + result = drm_edid_block_check_error(*blockp, 0, last_error_flags); + if (result) { + kfree(*blockp); + *blockp = NULL; + } } - if (!ret) { + if (!result) { n_blocks--; if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks) n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
while (n_blocks--) { - if (drm_edid_block_valid(*blockp + (valid_extensions + 1) * EDID_LENGTH, - valid_extensions + 1, print_bad_edid)) + if (!drm_edid_block_check_error(*blockp + (valid_extensions + 1) * EDID_LENGTH, + valid_extensions + 1, last_error_flags)) valid_extensions++; } fixup_edid(blockp, valid_extensions); - } else - connector->bad_edid_counter++; + ret = 0; + } + connector->last_edid_error_flags = result; return ret; }
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index b884c36..e80ba63 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -961,7 +961,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) drm_get_connector_name(connector)); /* rs690 seems to have a problem with connectors not existing and always * return a block of 0's. If we see this just stop polling on this output */ - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) { + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.last_edid_error_flags & EDID_ERR_NULL) { ret = connector_status_disconnected; DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector)); radeon_connector->ddc_bus = NULL; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6a1054c..171aa33 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -608,8 +608,7 @@ struct drm_connector { bool latency_present[2]; int video_latency[2]; /* [0]: progressive, [1]: interlaced */ int audio_latency[2]; - int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */ - unsigned bad_edid_counter; + unsigned last_edid_error_flags; };
/** @@ -1056,7 +1055,6 @@ extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); extern bool drm_edid_is_valid(struct edid *edid); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c880510..bf3b5d5 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -244,6 +244,14 @@ struct edid {
#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
+enum edid_error { + EDID_ERR_NO_BLOCK0 = 1 << 0, + EDID_ERR_NULL = 1 << 1, + EDID_ERR_CSUM = 1 << 2, + EDID_ERR_UNSUPPORTED_VERSION = 1 << 3, + EDID_ERR_NO_DATA = 1 << 4, +}; + struct drm_encoder; struct drm_connector; struct drm_display_mode; @@ -257,5 +265,6 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); void drm_cache_edid(struct drm_connector *connector, struct edid *edid); +unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
#endif /* __DRM_EDID_H__ */
So far it was only possible to load an EDID for a single connector (unless no connector was specified at all in which case the same EDID file was used for all). This patch extends the EDID loader so that EDID files can be specified for more than one connector. A semicolon is used to separate the different connector sections. The option now looks like this: edid_firmware="[<connector_0>:]<edid_file_0>[;<connector_n>:<edid_file_1>]..."
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/drm_edid_load.c | 45 ++++++++++++++++++++++++++------------ 1 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 1475c6f..a40a88505 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -196,26 +196,43 @@ struct edid * drm_load_edid_firmware(struct drm_connector *connector) { char *connector_name = drm_get_connector_name(connector); - char *edidname = edid_firmware, *last, *colon; - struct edid *edid; + char *edidname, *last, *colon; + struct edid *edid = NULL; + char *next, *mem; + bool many = false;
- if (*edidname == '\0') + if (*edid_firmware == '\0') return NULL;
- colon = strchr(edidname, ':'); - if (colon != NULL) { - if (strncmp(connector_name, edidname, colon - edidname)) - return NULL; - edidname = colon + 1; - if (*edidname == '\0') - return NULL; + edidname = mem = kstrndup(edid_firmware, PATH_MAX, GFP_KERNEL); + do { + next = strchr(edidname, ';'); + if (next) + *(next++) = '\0'; + colon = strchr(edidname, ':'); + if (colon == NULL) { + if (next || many) + edidname = NULL; + break; + } else if (!strncmp(connector_name, edidname, colon - edidname)) { + edidname = colon + 1; + break; + } + edidname = next; + many = true; + } while (edidname); + + if (edidname && *edidname != '\0') { + last = edidname + strlen(edidname) - 1; + if (*last == '\n') + *last = '\0'; + + if (strlen(edidname) > 0) + edid = edid_load(connector, edidname, connector_name); }
- last = edidname + strlen(edidname) - 1; - if (*last == '\n') - *last = '\0'; + kfree(mem);
- edid = edid_load(connector, edidname, connector_name); if (IS_ERR_OR_NULL(edid)) return NULL;
At Mon, 19 Nov 2012 15:23:14 -0500, "Egbert Eich " <"eich@novell.com> wrote:
So far it was only possible to load an EDID for a single connector (unless no connector was specified at all in which case the same EDID file was used for all). This patch extends the EDID loader so that EDID files can be specified for more than one connector. A semicolon is used to separate the different connector sections. The option now looks like this: edid_firmware="[<connector_0>:]<edid_file_0>[;<connector_n>:<edid_file_1>]..."
Signed-off-by: Egbert Eich eich@suse.de
drivers/gpu/drm/drm_edid_load.c | 45 ++++++++++++++++++++++++++------------ 1 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 1475c6f..a40a88505 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -196,26 +196,43 @@ struct edid * drm_load_edid_firmware(struct drm_connector *connector) { char *connector_name = drm_get_connector_name(connector);
- char *edidname = edid_firmware, *last, *colon;
- struct edid *edid;
- char *edidname, *last, *colon;
- struct edid *edid = NULL;
- char *next, *mem;
- bool many = false;
- if (*edidname == '\0')
- if (*edid_firmware == '\0') return NULL;
- colon = strchr(edidname, ':');
- if (colon != NULL) {
if (strncmp(connector_name, edidname, colon - edidname))return NULL;edidname = colon + 1;if (*edidname == '\0')return NULL;
- edidname = mem = kstrndup(edid_firmware, PATH_MAX, GFP_KERNEL);
- do {
Missing NULL check of edidname.
Takashi
next = strchr(edidname, ';');if (next)*(next++) = '\0';colon = strchr(edidname, ':');if (colon == NULL) {if (next || many)edidname = NULL;break;} else if (!strncmp(connector_name, edidname, colon - edidname)) {edidname = colon + 1;break;}edidname = next;many = true;- } while (edidname);
- if (edidname && *edidname != '\0') {
last = edidname + strlen(edidname) - 1;if (*last == '\n')*last = '\0';if (strlen(edidname) > 0)edid = edid_load(connector, edidname, connector_name);
- last = edidname + strlen(edidname) - 1;
- if (*last == '\n')
*last = '\0';
- kfree(mem);
- edid = edid_load(connector, edidname, connector_name); if (IS_ERR_OR_NULL(edid)) return NULL;
-- 1.7.7
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 7fc9f72..c27aa8d 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -31,6 +31,7 @@ #include <drm/drmP.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h> #include "ast_drv.h"
#include "ast_tables.h"
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/gma500/cdv_intel_dp.c | 1 + drivers/gpu/drm/gma500/oaktrail_lvds.c | 1 + drivers/gpu/drm/gma500/psb_intel_modes.c | 1 + 3 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c index e3a3978..37cad73 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_dp.c +++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c @@ -34,6 +34,7 @@ #include "psb_intel_drv.h" #include "psb_intel_reg.h" #include <drm/drm_dp_helper.h> +#include <drm/drm_edid.h>
#define _wait_for(COND, MS, W) ({ \ unsigned long timeout__ = jiffies + msecs_to_jiffies(MS); \ diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c index 558c77f..f7e20f5 100644 --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c @@ -22,6 +22,7 @@
#include <linux/i2c.h> #include <drm/drmP.h> +#include <drm/drm_edid.h> #include <asm/mrst.h>
#include "intel_bios.h" diff --git a/drivers/gpu/drm/gma500/psb_intel_modes.c b/drivers/gpu/drm/gma500/psb_intel_modes.c index 4fca0d6..1da014d 100644 --- a/drivers/gpu/drm/gma500/psb_intel_modes.c +++ b/drivers/gpu/drm/gma500/psb_intel_modes.c @@ -20,6 +20,7 @@ #include <linux/i2c.h> #include <linux/fb.h> #include <drm/drmP.h> +#include <drm/drm_edid.h> #include "psb_intel_drv.h"
/**
Signed-off-by: Egbert Eich eich@suse.de --- drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index d3d99a2..f89a0c1 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -15,6 +15,7 @@
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h>
#include "mgag200_drv.h"
Signed-off-by: Egbert Eich eich@suse.de --- include/drm/drm_crtc.h | 8 -------- include/drm/drm_edid.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 171aa33..06254bc 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -878,9 +878,6 @@ extern char *drm_get_tv_subconnector_name(int val); extern char *drm_get_tv_select_name(int val); extern void drm_fb_release(struct drm_file *file_priv); extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group); -extern bool drm_probe_ddc(struct i2c_adapter *adapter); -extern struct edid *drm_get_edid(struct drm_connector *connector, - struct i2c_adapter *adapter); extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid); extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode); extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode); @@ -1036,9 +1033,6 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern int drm_mode_gamma_set_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -extern u8 *drm_find_cea_extension(struct edid *edid); -extern bool drm_detect_hdmi_monitor(struct edid *edid); -extern bool drm_detect_monitor_audio(struct edid *edid); extern int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, @@ -1054,8 +1048,6 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev, extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
-extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern bool drm_edid_is_valid(struct edid *edid); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, bool rb); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index bf3b5d5..a18a8b9 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -267,4 +267,13 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len void drm_cache_edid(struct drm_connector *connector, struct edid *edid); unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
+extern bool drm_probe_ddc(struct i2c_adapter *adapter); +extern struct edid *drm_get_edid(struct drm_connector *connector, + struct i2c_adapter *adapter); +extern u8 *drm_find_cea_extension(struct edid *edid); +extern bool drm_detect_hdmi_monitor(struct edid *edid); +extern bool drm_detect_monitor_audio(struct edid *edid); +extern int drm_edid_header_is_valid(const u8 *raw_edid); +extern bool drm_edid_is_valid(struct edid *edid); + #endif /* __DRM_EDID_H__ */
The patches have been reordered and the changes suggested by Takashi Iwai have been worked in.
Egbert Eich (18):
1. Make error handling of EDID extension blocks a bit more fault tolorant: * Don't fail when EDID extension blocks cannot be read or memory cannot be allocated. * Don't read EDID extension blocks there are no blocks to expect, either for EDID versions < 1.3 or when block 3 is equal to the base block (in which case the monitor is not EDDC capable and doesn't accept the segment address.
DRM/KMS/EDID: Mask out Segment Bits when calculating Offset. DRM/KMS/EDID: 0x7e -> EDID_EXTENSION_FLAG_OFFSET (v2) DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails (v2) DRM/KMS/EDID: Don't fail when failing to allocate memory for EDID extensions. DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2) DRM/KMS/EDID: Don't expect extension blocks for EDID Versions < 1.3. DRM/KMS/EDID: Don't reallocate EDID blob when size has shrunk.
2. Fix EDID block maps: We skip invalid EEDID blocks, if we do so however we should fix the block maps to keep the EDID consistent.
DRM/KMS/EDID: Fix up EEDID Map Blogs if Extension Block Count has changed (v2)
3. Improve handling of 'firmware'-supplied EDIDs: * Move EDID loading from a helper level to DRM core. * If there is a 'firmware'-supplied EDID matching the connector supply it whenever drm_get_edid() is called. * Allow to specify firmware for multiple connectors. * Use the same EDID fault handling as for DDC read EDIDs.
DRM/KMS/EDID: Move drm_edid_load.o to drm.ko (v2) DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID is read (v2) DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files (v2) DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID (v2)
4. Cache EDIDs with extension blocks. I2C transfer of EDIDs with several extension blocks can be quite time consuming. In many cases the EDID has not changed for a given connector since the last read. We detect this by comparing data from the base block. If a match is found we copy the EDID data from cache. This way only the base block needs to be transferred.
DRM/KMS/EDID: Cache EDID blobs with extensions (v2)
5. Consolidate EDID fatal error handling: Store information about the error status of the last DDC read in the drm_connector structure. It combines the handling of all zero EDIDs (needed by the radeon driver) and the suppression of consecutive logging of the same EDID error. It will log again when the error pattern has changed (for inststance if the monitor has been exchanged).
DRM/KMS/EDID: Consolidate EDID Error Handling (v2)
6. Move EDID-related functions to drm_edid.h The handling of EDID related DRM functions seemed to be a bit arbitrary: although drm_edid.h had been created, new functions were often added to drm_crtc.h
DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid(). DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid(). DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid(). DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.
drivers/gpu/drm/Kconfig | 3 +- drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/ast/ast_mode.c | 1 + drivers/gpu/drm/drm_crtc.c | 1 + drivers/gpu/drm/drm_crtc_helper.c | 6 +- drivers/gpu/drm/drm_edid.c | 333 ++++++++++++++++++++++++---- drivers/gpu/drm/drm_edid_load.c | 115 ++++------ drivers/gpu/drm/gma500/cdv_intel_dp.c | 1 + drivers/gpu/drm/gma500/oaktrail_lvds.c | 1 + drivers/gpu/drm/gma500/psb_intel_modes.c | 1 + drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 13 +- include/drm/drm_edid.h | 25 ++- 14 files changed, 370 insertions(+), 135 deletions(-)
This patch is a bit cosmetic as the variable size will truncate the start address anyway but for readability it should be made explicite that the lowest bit in the EDID block number determines the I2C start address.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index fadcd44..049fa52 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -258,7 +258,7 @@ static int drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, int block, int len) { - unsigned char start = block * EDID_LENGTH; + unsigned char start = (block & 0x01) * EDID_LENGTH; unsigned char segment = block >> 1; unsigned char xfers = segment ? 3 : 2; int ret, retries = 5;
v2: Use offsetof().
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 16 +++++++++------- 1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 049fa52..9e64069 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -43,6 +43,8 @@ #define EDID_STD_TIMINGS 8 #define EDID_DETAILED_TIMINGS 4
+#define EDID_EXTENSION_FLAG_OFFSET offsetof(struct edid, extensions) +#define EDID_CHECKSUM_OFFSET offsetof(struct edid, checksum) /* * EDID blocks out in the wild have a variety of bugs, try to collect * them here (note that userspace may work around broken monitors first, @@ -341,15 +343,15 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) goto carp;
/* if there's no extensions, we're done */ - if (block[0x7e] == 0) + if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) return block;
- new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); + new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; block = new;
- for (j = 1; j <= block[0x7e]; j++) { + for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) { for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block + (valid_extensions + 1) * EDID_LENGTH, @@ -366,9 +368,9 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) drm_get_connector_name(connector), j); }
- if (valid_extensions != block[0x7e]) { - block[EDID_LENGTH-1] += block[0x7e] - valid_extensions; - block[0x7e] = valid_extensions; + if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { + block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; + block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; @@ -601,7 +603,7 @@ drm_for_each_detailed_block(u8 *raw_edid, detailed_cb *cb, void *closure) for (i = 0; i < EDID_DETAILED_TIMINGS; i++) cb(&(edid->detailed_timings[i]), closure);
- for (i = 1; i <= raw_edid[0x7e]; i++) { + for (i = 1; i <= raw_edid[EDID_EXTENSION_FLAG_OFFSET]; i++) { u8 *ext = raw_edid + (i * EDID_LENGTH); switch (*ext) { case CEA_EXT:
If I2C readout fails for an extension block but we have read a valid base block, don't fail completely but at least return the base block.
v2: Make goto target names more telling.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 9e64069..043ba42 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -355,8 +355,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block + (valid_extensions + 1) * EDID_LENGTH, - j, EDID_LENGTH)) - goto out; + j, EDID_LENGTH)) { + valid_extensions = 0; + goto done_fix_extension_count; + } if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { valid_extensions++; break; @@ -368,6 +370,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) drm_get_connector_name(connector), j); }
+done_fix_extension_count: if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions;
When we fail to allocate space for EDID extensions we should just warn, fix up the EDID block count and return the base block instead of failing.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 043ba42..a952cfe 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -347,8 +347,11 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) return block;
new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); - if (!new) - goto out; + if (!new) { + dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n", + drm_get_connector_name(connector), block[EDID_EXTENSION_FLAG_OFFSET] + 1); + goto done_fix_extension_count; + } block = new;
for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) {
There are displays which announce EDID extension blocks in the Extension Flag of the EDID base block although they are not EDDC capable (ie. take a segment address at I2C slave address 0x30). We test this by looking for an EDID header which is only possible in the base block. If the segment address is not taken into account, this block will be identical to the base block in which case we stop reading further EEDID blocks, correct the extension flag and just return the base block.
v2: Split up EDID fixup code into separate commit.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a952cfe..5a0e331 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) } if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { valid_extensions++; + /* Test if base block announced extension blocks although + * display is not EDDC capable. + */ + if (j == 2) { + int k; + for (k = 0; k < sizeof(edid_header); k++) + if (block[(EDID_LENGTH * 2) + k] != edid_header[k]) + break; + if (k == sizeof(edid_header)) { + valid_extensions = 0; + goto done_fix_extension_count; + } + } break; } }
On Thu, Nov 22, 2012 at 05:22:55AM -0500, Egbert Eich wrote:
There are displays which announce EDID extension blocks in the Extension Flag of the EDID base block although they are not EDDC capable (ie. take a segment address at I2C slave address 0x30). We test this by looking for an EDID header which is only possible in the base block. If the segment address is not taken into account, this block will be identical to the base block in which case we stop reading further EEDID blocks, correct the extension flag and just return the base block.
v2: Split up EDID fixup code into separate commit.
Signed-off-by: Egbert Eich eich@suse.com
drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a952cfe..5a0e331 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -364,6 +364,19 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) } if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { valid_extensions++;
/* Test if base block announced extension blocks although* display is not EDDC capable.*/if (j == 2) {int k;for (k = 0; k < sizeof(edid_header); k++)if (block[(EDID_LENGTH * 2) + k] != edid_header[k])break;if (k == sizeof(edid_header)) {valid_extensions = 0;goto done_fix_extension_count;}
memcmp()? Also couldn't we just memcmp() the whole block against the base block, instead of just the header part?
Also the comment is somehow misleading. It talks about the base block even though we're looking at the extension block.
There are displays which announce EDID extension blocks in the Extension Flag of the EDID base block although they are not EDDC capable (ie. take a segment address at I2C slave address 0x30). We test this by looking for an EDID header which is only possible in the base block. If the segment address is not taken into account, this block will be identical to the base block in which case we stop reading further EEDID blocks, correct the extension flag and just return the base block.
v2: Split up EDID fixup code into separate commit. v3: Worked in changes suggested by Ville Syrjälä ville.syrjala@linux.intel.com: Reworded comment, Used memcmp(), Compared entire base block instead of signature only.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a952cfe..b42fbc0 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -364,6 +364,16 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) } if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { valid_extensions++; + /* If extension block 2 is identical to the base block the display is probably + * not EDDC cabable - despite of what the extension flag says - as it doesn't + * select the correct segment address: detect this condition and bail early. + */ + if (j == 2) { + if (memcmp(block + EDID_LENGTH * 2, block, EDID_LENGTH) == 0) { + valid_extensions = 0; + goto done_fix_extension_count; + } + } break; } }
EDID extension blogs are only expected for EDIDs version 1.3 or higher. If an EDID with a lower version is found fix the block count in the extension flags and return the base block. This should help to avoid issues with older displays with broken DDC implementations.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 5a0e331..da2f7fa 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -45,6 +45,8 @@
#define EDID_EXTENSION_FLAG_OFFSET offsetof(struct edid, extensions) #define EDID_CHECKSUM_OFFSET offsetof(struct edid, checksum) +#define EDID_VERSION_MAJOR_OFFSET offsetof(struct edid, version) +#define EDID_VERSION_MINOR_OFFSET offsetof(struct edid, revision) /* * EDID blocks out in the wild have a variety of bugs, try to collect * them here (note that userspace may work around broken monitors first, @@ -346,6 +348,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) return block;
+ /* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */ + if (block[EDID_VERSION_MINOR_OFFSET] < 3) + goto done_fix_extension_count; + new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) { dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n",
valid_extensions (the number of EDID extensions found to be valid) can never be > block[EDID_EXTENSION_FLAG_OFFSET]. There is no point of reallocating the block in this case: the extra blocks at the end of the EDID structure will not hurt, also the implementation of krealloc() will just return the same block.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index da2f7fa..0fe61fb 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -396,10 +396,6 @@ done_fix_extension_count: if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; - new = krealloc(block, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); - if (!new) - goto out; - block = new; }
return block;
EEDID v1.3 mandates map blogs if more than one EDID extension block is used while in v1.4 they are optional. If the extension count has been changed (because some extension blocks were not readable) those map blocks need fixing. In case of v1.4 or higher we simply eliminate all map blogs as this is less time consuming. For v1.3 we scrap any exsisting map blocks and recreate them from scratch.
v2: Fixed conflits due to reordering of commits.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 89 ++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 89 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 0fe61fb..9b298fc 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -47,6 +47,7 @@ #define EDID_CHECKSUM_OFFSET offsetof(struct edid, checksum) #define EDID_VERSION_MAJOR_OFFSET offsetof(struct edid, version) #define EDID_VERSION_MINOR_OFFSET offsetof(struct edid, revision) +#define EEDID_BLOCK_MAP_FLAG 0xF0 /* * EDID blocks out in the wild have a variety of bugs, try to collect * them here (note that userspace may work around broken monitors first, @@ -320,6 +321,92 @@ static bool drm_edid_is_zero(u8 *in_edid, int length) return true; }
+static void +fix_map(u8 *block, int cnt) +{ + int i; + u8 csum; + + if (--cnt > 127) + cnt = 127; + memset(block, 0, 128); + block[0] = EEDID_BLOCK_MAP_FLAG; + csum = block[0]; + + for (i = 1; i < cnt; i++) { + block[i] = block[i * EDID_LENGTH]; + csum += block[i]; + } + block[127] = (u8)(0x100 - csum); +} + +static int +fixup_blockmaps(u8 **blockp, int eblock_cnt) +{ + int i; + u8 *block = *blockp; + + if (block[EDID_VERSION_MAJOR_OFFSET] > 1) + return 0; + if (block[EDID_VERSION_MINOR_OFFSET] < 3) + return 0; + if (eblock_cnt == 1) { + if (block[EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) + return 0; + else + return 1; + } + if (block[EDID_VERSION_MINOR_OFFSET] >= 4) { + /* blockmaps are optional: simply toss them */ + for (i = 1; i <= eblock_cnt; i++) { + if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) { + if (i < eblock_cnt) + memmove(&block[i * EDID_LENGTH], + &block[(i + 1) * EDID_LENGTH], + (eblock_cnt-i) * EDID_LENGTH); + i--; + eblock_cnt--; + } + } + } else { + int total_cnt = block[EDID_EXTENSION_FLAG_OFFSET]; + for (i = 1; i <= eblock_cnt; i++) { + if (block[i * EDID_LENGTH] == EEDID_BLOCK_MAP_FLAG) { + if (i == 1 || i == 128) /* correct map block locations */ + continue; + if (i < eblock_cnt) + memmove(&block[i * EDID_LENGTH], + &block[(i + 1) * EDID_LENGTH], + (eblock_cnt-i) * EDID_LENGTH); + i--; + eblock_cnt--; + continue; + } else if (i == 1 || i == 128) { + if (eblock_cnt >= total_cnt) { + u8 *tmp_p; + tmp_p = krealloc(block, (eblock_cnt + 2) * EDID_LENGTH, GFP_KERNEL); + if (!tmp_p) + return eblock_cnt; + *blockp = block = tmp_p; + total_cnt = eblock_cnt + 1; + } + eblock_cnt++; + memmove(&block[(i + 1) * EDID_LENGTH], + &block[i * EDID_LENGTH], + (eblock_cnt-i) * EDID_LENGTH); + } + } + fix_map(&block[EDID_LENGTH], eblock_cnt); + + if (eblock_cnt == 129) + return 128; + + if (eblock_cnt > 129) + fix_map(&block[EDID_LENGTH * 128], eblock_cnt - 127); + } + return eblock_cnt; +} + static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -394,6 +481,8 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
done_fix_extension_count: if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { + if (valid_extensions) + valid_extensions = fixup_blockmaps(&block, valid_extensions); block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; }
EDIDs are an integral concept of connectors, connectors are a concept of drm core also drm_edid.o is already part of this drm core. Overridden, 'firmware-supplied' EDIDs should be treated exactly the same as device supplied ones. Therefore move drm_edid_load from the helper level to the drm core.
v2: a. Also adapt Kconfig b. Add missing 'select FW_LOADER'
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/Kconfig | 3 ++- drivers/gpu/drm/Makefile | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 18321b68b..4bc239b 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -36,7 +36,8 @@ config DRM_KMS_HELPER
config DRM_LOAD_EDID_FIRMWARE bool "Allow to specify an EDID data set instead of probing for it" - depends on DRM_KMS_HELPER + depends on DRM + select FW_LOADER help Say Y here, if you want to use EDID data to be loaded from the /lib/firmware directory or one of the provided built-in diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 2ff5cef..76ca269 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -16,11 +16,11 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm-usb-y := drm_usb.o
drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
Firmware supplied EDIDs where fed in in drm_helper_probe_single_connector_modes() in place of calling the driver supplied get_modes() function. This has two problems: 1. Drivers don't call drm_get_edid() only from within get_modes(). 2. The get_modes() replacement in drm_load_edid_firmware() provided only the least common denominator of what driver provided get_modes() callbacks do. This patch now supplies a user provided 'firmware' EDID whenever drm_get_edid() is called if one is available. The user supplied EDID is thus used the same way as a display provided one (except for detecting the presence of a device thru EDID). Also it does so not on the helper level any more, the possibility for EDID loading now happens on the DRM KMS core level.
v2: Fixed formatting and conflicts due to reordering of commits.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_crtc_helper.c | 6 +----- drivers/gpu/drm/drm_edid.c | 7 +++++++ drivers/gpu/drm/drm_edid_load.c | 22 +++++++++------------- include/drm/drm_edid.h | 4 +++- 4 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 1227adf..bc99595 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -119,11 +119,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, goto prune; }
-#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE - count = drm_load_edid_firmware(connector); - if (count == 0) -#endif - count = (*connector_funcs->get_modes)(connector); + count = (*connector_funcs->get_modes)(connector);
if (count == 0 && connector->status == connector_status_connected) count = drm_add_modes_noedid(connector, 1024, 768); diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 9b298fc..8239c42 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -414,6 +414,13 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) u8 *block, *new; bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
+#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE + /* check if the user has specified a 'firmware' EDID file */ + block = (u8 *)drm_load_edid_firmware(connector); + if (block) + return block; +#endif + if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) return NULL;
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 38d3943..748f63b 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -114,7 +114,7 @@ static u8 generic_edid[GENERIC_EDIDS][128] = { }, };
-static u8 *edid_load(struct drm_connector *connector, char *name, +static struct edid *edid_load(struct drm_connector *connector, char *name, char *connector_name) { const struct firmware *fw; @@ -222,36 +222,32 @@ out: return edid; }
-int drm_load_edid_firmware(struct drm_connector *connector) +struct edid * +drm_load_edid_firmware(struct drm_connector *connector) { char *connector_name = drm_get_connector_name(connector); char *edidname = edid_firmware, *last, *colon; - int ret; struct edid *edid;
if (*edidname == '\0') - return 0; + return NULL;
colon = strchr(edidname, ':'); if (colon != NULL) { if (strncmp(connector_name, edidname, colon - edidname)) - return 0; + return NULL; edidname = colon + 1; if (*edidname == '\0') - return 0; + return NULL; }
last = edidname + strlen(edidname) - 1; if (*last == '\n') *last = '\0';
- edid = (struct edid *) edid_load(connector, edidname, connector_name); + edid = edid_load(connector, edidname, connector_name); if (IS_ERR_OR_NULL(edid)) - return 0; + return NULL;
- drm_mode_connector_update_edid_property(connector, edid); - ret = drm_add_edid_modes(connector, edid); - kfree(edid); - - return ret; + return edid; } diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 0cac551..c0a77bd 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -252,6 +252,8 @@ int drm_av_sync_delay(struct drm_connector *connector, struct drm_display_mode *mode); struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct drm_display_mode *mode); -int drm_load_edid_firmware(struct drm_connector *connector); +#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE +struct edid *drm_load_edid_firmware(struct drm_connector *connector); +#endif
#endif /* __DRM_EDID_H__ */
So far it was only possible to load an EDID for a single connector (unless no connector was specified at all in which case the same EDID file was used for all). This patch extends the EDID loader so that EDID files can be specified for more than one connector. A semicolon is used to separate the different connector sections. The option now looks like this: edid_firmware="[<connector_0>:]<edid_file_0>[;<connector_n>:<edid_file_1>]..."
v2: Check for edidname != NULL before entering loop.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid_load.c | 45 ++++++++++++++++++++++++++------------ 1 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 748f63b..9c15c4a 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -226,26 +226,43 @@ struct edid * drm_load_edid_firmware(struct drm_connector *connector) { char *connector_name = drm_get_connector_name(connector); - char *edidname = edid_firmware, *last, *colon; - struct edid *edid; + char *edidname, *last, *colon; + struct edid *edid = NULL; + char *next, *mem; + bool many = false;
- if (*edidname == '\0') + if (*edid_firmware == '\0') return NULL;
- colon = strchr(edidname, ':'); - if (colon != NULL) { - if (strncmp(connector_name, edidname, colon - edidname)) - return NULL; - edidname = colon + 1; - if (*edidname == '\0') - return NULL; + edidname = mem = kstrndup(edid_firmware, PATH_MAX, GFP_KERNEL); + while (edidname) { + next = strchr(edidname, ';'); + if (next) + *(next++) = '\0'; + colon = strchr(edidname, ':'); + if (colon == NULL) { + if (next || many) + edidname = NULL; + break; + } else if (!strncmp(connector_name, edidname, colon - edidname)) { + edidname = colon + 1; + break; + } + edidname = next; + many = true; + } + + if (edidname && *edidname != '\0') { + last = edidname + strlen(edidname) - 1; + if (*last == '\n') + *last = '\0'; + + if (strlen(edidname) > 0) + edid = edid_load(connector, edidname, connector_name); }
- last = edidname + strlen(edidname) - 1; - if (*last == '\n') - *last = '\0'; + kfree(mem);
- edid = edid_load(connector, edidname, connector_name); if (IS_ERR_OR_NULL(edid)) return NULL;
in drm_edid.c there's now code to fix extension blockmaps if the number of extensions has changed. This code also rearranges the EDID blocks. Replace the exisiting EDID rearrange code with a call to this code.
v2: Make adjustments required by patch reordering, add missing memcpy().
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 64 +++++++++++++++++++++++++++++++++++---- drivers/gpu/drm/drm_edid_load.c | 54 +++++++------------------------- include/drm/drm_edid.h | 1 + 3 files changed, 71 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 8239c42..a47fa7f 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -407,6 +407,18 @@ fixup_blockmaps(u8 **blockp, int eblock_cnt) return eblock_cnt; }
+static void +fixup_edid(u8 **blockp, int valid_extensions) +{ + if (valid_extensions != (*blockp)[EDID_EXTENSION_FLAG_OFFSET]) { + if (valid_extensions) + valid_extensions = fixup_blockmaps(blockp, valid_extensions); + + (*blockp)[EDID_CHECKSUM_OFFSET] += (*blockp)[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; + (*blockp)[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; + } +} + static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -487,12 +499,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) }
done_fix_extension_count: - if (valid_extensions != block[EDID_EXTENSION_FLAG_OFFSET]) { - if (valid_extensions) - valid_extensions = fixup_blockmaps(&block, valid_extensions); - block[EDID_CHECKSUM_OFFSET] += block[EDID_EXTENSION_FLAG_OFFSET] - valid_extensions; - block[EDID_EXTENSION_FLAG_OFFSET] = valid_extensions; - } + fixup_edid(&block, valid_extensions);
return block;
@@ -509,6 +516,51 @@ out: }
/** + * Validate an entire EDID blob. + * \param connector: drm_connector struct of the used connector. + * \param blockp: pointer to address of an raw EDID data block. + * \param len: size if block in bytes. + * + * validate block and return corrected block in \param block. + * \return: number of valid extensions or -errno if unsuccessful. + */ +int +drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) +{ + int n_blocks = len / EDID_LENGTH; + int valid_extensions = 0, ret = 0; + bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + + if (!blockp || !*blockp) + ret = -EINVAL; + else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) { + kfree(*blockp); + *blockp = NULL; + ret = -EINVAL; + } + if (!ret) { + int cnt = 0; + n_blocks--; + if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks) + n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET]; + + while (n_blocks--) { + cnt++; + if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH, + valid_extensions + 1, print_bad_edid)) { + valid_extensions++; + if (cnt != valid_extensions) + memcpy(*blockp + valid_extensions * EDID_LENGTH, + *blockp + cnt * EDID_LENGTH, EDID_LENGTH); + } + } + fixup_edid(blockp, valid_extensions); + } else + connector->bad_edid_counter++; + return ret; +} + +/** * Probe DDC presence. * * \param adapter : i2c device adaptor diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c index 9c15c4a..8f26790 100644 --- a/drivers/gpu/drm/drm_edid_load.c +++ b/drivers/gpu/drm/drm_edid_load.c @@ -119,11 +119,10 @@ static struct edid *edid_load(struct drm_connector *connector, char *name, { const struct firmware *fw; struct platform_device *pdev; - u8 *fwdata = NULL, *edid, *new_edid; + u8 *fwdata = NULL; + struct edid *edid; int fwsize, expected; int builtin = 0, err = 0; - int i, valid_extensions = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
pdev = platform_device_register_simple(connector_name, -1, NULL, 0); if (IS_ERR(pdev)) { @@ -137,7 +136,7 @@ static struct edid *edid_load(struct drm_connector *connector, char *name, platform_device_unregister(pdev);
if (err) { - i = 0; + int i = 0; while (i < GENERIC_EDIDS && strcmp(name, generic_edid_name[i])) i++; if (i < GENERIC_EDIDS) { @@ -174,49 +173,20 @@ static struct edid *edid_load(struct drm_connector *connector, char *name, } memcpy(edid, fwdata, fwsize);
- if (!drm_edid_block_valid(edid, 0, print_bad_edid)) { - connector->bad_edid_counter++; - DRM_ERROR("Base block of EDID firmware "%s" is invalid ", - name); - kfree(edid); - err = -EINVAL; - goto relfw_out; - } - - for (i = 1; i <= edid[0x7e]; i++) { - if (i != valid_extensions + 1) - memcpy(edid + (valid_extensions + 1) * EDID_LENGTH, - edid + i * EDID_LENGTH, EDID_LENGTH); - if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid)) - valid_extensions++; - } - - if (valid_extensions != edid[0x7e]) { - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; - DRM_INFO("Found %d valid extensions instead of %d in EDID data " - ""%s" for connector "%s"\n", valid_extensions, - edid[0x7e], name, connector_name); - edid[0x7e] = valid_extensions; - new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, - GFP_KERNEL); - if (new_edid == NULL) { - err = -ENOMEM; - kfree(edid); - goto relfw_out; - } - edid = new_edid; - } - - DRM_INFO("Got %s EDID base block and %d extension%s from " - ""%s" for connector "%s"\n", builtin ? "built-in" : - "external", valid_extensions, valid_extensions == 1 ? "" : "s", - name, connector_name); + err = drm_validate_edid_blob(connector, (u8 **)&edid, fwsize); + if (err < 0) + DRM_ERROR("EDID firmware "%s" is invalid ", name); + else + DRM_INFO("Got %s EDID base block and %d extension%s from " + ""%s" for connector "%s"\n", builtin ? "built-in" : + "external", edid->extensions, edid->extensions == 1 ? "" : "s", + name, connector_name);
relfw_out: release_firmware(fw);
out: - if (err) + if (err < 0) return ERR_PTR(err);
return edid; diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c0a77bd..53c619c 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -255,5 +255,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder, #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif +int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len);
#endif /* __DRM_EDID_H__ */
According the the VESA specs there can be up to 254 EEDID extension blocks. Since we may read the EDID (including extensions) in 10 second intervals to probe for display hotplugging (at least in cases where no hardware hotplug detection exists) and I2C transfer is rather slow we may end up consuming a considerable amount on CPU time for just that. This patch caches the EDID block if it contains at least one extension. To determine if the blocks match we only tranfer the base block, on a match we use the cached data.
V2: Use kmemdup() instead of a kmalloc()/memcpy() combo, erase cache when reading a 'firmware'-supplied EDID or on error.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_crtc.c | 1 + drivers/gpu/drm/drm_edid.c | 57 +++++++++++++++++++++++++++++++++++++------ include/drm/drm_crtc.h | 1 + include/drm/drm_edid.h | 1 + 4 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3533609..e283355 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector) drm_mode_remove(connector, mode);
mutex_lock(&dev->mode_config.mutex); + drm_cache_edid(connector, NULL); drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--; diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a47fa7f..28b877c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -419,6 +419,38 @@ fixup_edid(u8 **blockp, int valid_extensions) } }
+static bool +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp) +{ + if (connector->edid_cache && + connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] && + connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] && + connector->edid_cache->serial == (*edidp)->serial && + connector->edid_cache->input == (*edidp)->input) { + int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH; + struct edid *new = kmemdup(connector->edid_cache, size, GFP_KERNEL); + if (!new) + return false; + DRM_DEBUG_KMS("Got EDID for %s from cache.\n", drm_get_connector_name(connector)); + kfree(*edidp); + *edidp = new; + return true; + } + return false; +} + +void +drm_cache_edid(struct drm_connector *connector, struct edid *edid) +{ + struct edid *new = NULL; + kfree(connector->edid_cache); + if (edid) { + int size = (edid->extensions + 1) * EDID_LENGTH; + new = kmemdup(edid, size, GFP_KERNEL); + } + connector->edid_cache = new; +} + static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -429,28 +461,30 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector); - if (block) + if (block) { + drm_cache_edid(connector, NULL); return block; + } #endif
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) - return NULL; + goto error;
/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) - goto out; + goto error_free; if (drm_edid_block_valid(block, 0, print_bad_edid)) break; if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { connector->null_edid_counter++; - goto carp; + goto error_carp; } } if (i == 4) - goto carp; + goto error_carp;
- /* if there's no extensions, we're done */ + /* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) return block;
@@ -458,6 +492,10 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) if (block[EDID_VERSION_MINOR_OFFSET] < 3) goto done_fix_extension_count;
+ /* see if EDID is in the cache - no need to read all extension blocks */ + if (compare_get_edid_from_cache(connector, (struct edid **)&block)) + return block; + new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) { dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n", @@ -500,18 +538,21 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
done_fix_extension_count: fixup_edid(&block, valid_extensions); + drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL);
return block;
-carp: +error_carp: if (print_bad_edid) { dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } connector->bad_edid_counter++;
-out: +error_free: kfree(block); +error: + drm_cache_edid(connector, NULL); return NULL; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 49dd8c2..6a1054c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -601,6 +601,7 @@ struct drm_connector { struct drm_encoder *encoder; /* currently active encoder */
/* EDID bits */ + struct edid *edid_cache; uint8_t eld[MAX_ELD_BYTES]; bool dvi_dual; int max_tmds_clock; /* in MHz */ diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 53c619c..c880510 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); +void drm_cache_edid(struct drm_connector *connector, struct edid *edid);
#endif /* __DRM_EDID_H__ */
On Thu, Nov 22, 2012 at 05:23:03AM -0500, Egbert Eich wrote:
According the the VESA specs there can be up to 254 EEDID extension blocks. Since we may read the EDID (including extensions) in 10 second intervals to probe for display hotplugging (at least in cases where no hardware hotplug detection exists) and I2C transfer is rather slow we may end up consuming a considerable amount on CPU time for just that. This patch caches the EDID block if it contains at least one extension. To determine if the blocks match we only tranfer the base block, on a match we use the cached data.
V2: Use kmemdup() instead of a kmalloc()/memcpy() combo, erase cache when reading a 'firmware'-supplied EDID or on error.
Signed-off-by: Egbert Eich eich@suse.com
drivers/gpu/drm/drm_crtc.c | 1 + drivers/gpu/drm/drm_edid.c | 57 +++++++++++++++++++++++++++++++++++++------ include/drm/drm_crtc.h | 1 + include/drm/drm_edid.h | 1 + 4 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3533609..e283355 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector) drm_mode_remove(connector, mode);
mutex_lock(&dev->mode_config.mutex);
- drm_cache_edid(connector, NULL); drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a47fa7f..28b877c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -419,6 +419,38 @@ fixup_edid(u8 **blockp, int valid_extensions) } }
+static bool +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp) +{
- if (connector->edid_cache &&
connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] &&connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] &&connector->edid_cache->serial == (*edidp)->serial &&connector->edid_cache->input == (*edidp)->input) {int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH;struct edid *new = kmemdup(connector->edid_cache, size, GFP_KERNEL);if (!new)return false;DRM_DEBUG_KMS("Got EDID for %s from cache.\n", drm_get_connector_name(connector));kfree(*edidp);*edidp = new;return true;- }
- return false;
+}
+void +drm_cache_edid(struct drm_connector *connector, struct edid *edid) +{
- struct edid *new = NULL;
- kfree(connector->edid_cache);
- if (edid) {
int size = (edid->extensions + 1) * EDID_LENGTH;new = kmemdup(edid, size, GFP_KERNEL);- }
- connector->edid_cache = new;
+}
static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -429,28 +461,30 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector);
- if (block)
- if (block) {
drm_cache_edid(connector, NULL);- }
#endif
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
return NULL;
goto error;/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
goto out;
goto error_free;
goto carp;
goto error_carp;
goto carp;
goto error_carp;
- /* if there's no extensions, we're done */
- /* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) return block;
Shouldn't you erase the cache here too?
According the the VESA specs there can be up to 254 EEDID extension blocks. Since we may read the EDID (including extensions) in 10 second intervals to probe for display hotplugging (at least in cases where no hardware hotplug detection exists) and I2C transfer is rather slow we may end up consuming a considerable amount on CPU time for just that. This patch caches the EDID block if it contains at least one extension. To determine if the blocks match we only tranfer the base block, on a match we use the cached data.
V2: Use kmemdup() instead of a kmalloc()/memcpy() combo, erase cache when reading a 'firmware'-supplied EDID or on error. V3: Uncache when only one extension block is found. This chunk had accidentally gone into the next patch of the series. Found by Ville Syrjälä ville.syrjala@linux.intel.com.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_crtc.c | 1 + drivers/gpu/drm/drm_edid.c | 60 +++++++++++++++++++++++++++++++++++++------ include/drm/drm_crtc.h | 1 + include/drm/drm_edid.h | 1 + 4 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3533609..e283355 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -598,6 +598,7 @@ void drm_connector_cleanup(struct drm_connector *connector) drm_mode_remove(connector, mode);
mutex_lock(&dev->mode_config.mutex); + drm_cache_edid(connector, NULL); drm_mode_object_put(dev, &connector->base); list_del(&connector->head); dev->mode_config.num_connector--; diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index e269739..dd0df60 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -419,6 +419,38 @@ fixup_edid(u8 **blockp, int valid_extensions) } }
+static bool +compare_get_edid_from_cache(struct drm_connector *connector, struct edid **edidp) +{ + if (connector->edid_cache && + connector->edid_cache->prod_code[0] == (*edidp)->prod_code[0] && + connector->edid_cache->prod_code[1] == (*edidp)->prod_code[1] && + connector->edid_cache->serial == (*edidp)->serial && + connector->edid_cache->input == (*edidp)->input) { + int size = (connector->edid_cache->extensions + 1) * EDID_LENGTH; + struct edid *new = kmemdup(connector->edid_cache, size, GFP_KERNEL); + if (!new) + return false; + DRM_DEBUG_KMS("Got EDID for %s from cache.\n", drm_get_connector_name(connector)); + kfree(*edidp); + *edidp = new; + return true; + } + return false; +} + +void +drm_cache_edid(struct drm_connector *connector, struct edid *edid) +{ + struct edid *new = NULL; + kfree(connector->edid_cache); + if (edid) { + int size = (edid->extensions + 1) * EDID_LENGTH; + new = kmemdup(edid, size, GFP_KERNEL); + } + connector->edid_cache = new; +} + static u8 * drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { @@ -429,35 +461,41 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector); - if (block) + if (block) { + drm_cache_edid(connector, NULL); return block; + } #endif
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) - return NULL; + goto error;
/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) - goto out; + goto error_free; if (drm_edid_block_valid(block, 0, print_bad_edid)) break; if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { connector->null_edid_counter++; - goto carp; + goto error_carp; } } if (i == 4) - goto carp; + goto error_carp;
- /* if there's no extensions, we're done */ + /* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) - return block; + goto done;
/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */ if (block[EDID_VERSION_MINOR_OFFSET] < 3) goto done_fix_extension_count;
+ /* see if EDID is in the cache - no need to read all extension blocks */ + if (compare_get_edid_from_cache(connector, (struct edid **)&block)) + return block; + new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) { dev_warn(connector->dev->dev, "%s: cannot allocate memory for %d EDID blocks: truncating.\n", @@ -497,18 +535,22 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
done_fix_extension_count: fixup_edid(&block, valid_extensions); +done: + drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL);
return block;
-carp: +error_carp: if (print_bad_edid) { dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } connector->bad_edid_counter++;
-out: +error_free: kfree(block); +error: + drm_cache_edid(connector, NULL); return NULL; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 49dd8c2..6a1054c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -601,6 +601,7 @@ struct drm_connector { struct drm_encoder *encoder; /* currently active encoder */
/* EDID bits */ + struct edid *edid_cache; uint8_t eld[MAX_ELD_BYTES]; bool dvi_dual; int max_tmds_clock; /* in MHz */ diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 53c619c..c880510 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -256,5 +256,6 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder, struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); +void drm_cache_edid(struct drm_connector *connector, struct edid *edid);
#endif /* __DRM_EDID_H__ */
Consolidate the null_edid_counter and the bad_edid_counter into EDID error state flags which for the last EDID read are accessible from user. Errors are looged it the same error has not been present in the previous read of the EDID. This will reset the EDID error status for example when the monitor is changed but still prevents permanent EDID errors from piling up the the kernel logs.
v2: Fixed conflits due to reordering of commits. Set error state where missing.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 115 +++++++++++++++++----------- drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 4 +- include/drm/drm_edid.h | 10 +++ 4 files changed, 81 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 28b877c..7d8363e 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid) } EXPORT_SYMBOL(drm_edid_header_is_valid);
+static bool drm_edid_is_zero(u8 *in_edid, int length) +{ + int i; + u32 *raw_edid = (u32 *)in_edid; + + for (i = 0; i < length / 4; i++) + if (*(raw_edid + i) != 0) + return false; + return true; +} + static int edid_fixup __read_mostly = 6; module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, @@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup, * Sanity check the EDID block (base or extension). Return 0 if the block * doesn't check out, or 1 if it's valid. */ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) +unsigned +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags) { int i; u8 csum = 0; struct edid *edid = (struct edid *)raw_edid; + unsigned result = 0;
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6; @@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); memcpy(raw_edid, edid_header, sizeof(edid_header)); } else { - goto bad; + result |= EDID_ERR_NO_BLOCK0; + if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) { + result |= EDID_ERR_NULL; + goto bad; + } } }
for (i = 0; i < EDID_LENGTH; i++) csum += raw_edid[i]; if (csum) { - if (print_bad_edid) { + if ((last_error_flags & EDID_ERR_CSUM) == 0) DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum); - }
/* allow CEA to slide through, switches mangle this */ if (raw_edid[0] != 0x02) - goto bad; + result |= EDID_ERR_CSUM; } + if (result) + goto bad;
/* per-block-type checks */ switch (raw_edid[0]) { case 0: /* base */ if (edid->version != 1) { DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version); + result |= EDID_ERR_UNSUPPORTED_VERSION; goto bad; }
@@ -214,15 +233,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1; + return 0;
bad: - if (raw_edid && print_bad_edid) { + if (raw_edid && last_error_flags != result) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); } - return 0; + return result; +} + +bool +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags) +{ + if (!drm_edid_block_check_error(raw_edid, block, last_error_flags)) + return true; + return false; } EXPORT_SYMBOL(drm_edid_block_valid);
@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid) return false;
for (i = 0; i <= edid->extensions; i++) - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true)) + if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true)) return false;
return true; @@ -310,17 +337,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, return ret == xfers ? 0 : -1; }
-static bool drm_edid_is_zero(u8 *in_edid, int length) -{ - int i; - u32 *raw_edid = (u32 *)in_edid; - - for (i = 0; i < length / 4; i++) - if (*(raw_edid + i) != 0) - return false; - return true; -} - static void fix_map(u8 *block, int cnt) { @@ -456,37 +472,40 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { int i, j = 0, valid_extensions = 0; u8 *block, *new; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector); if (block) { drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = 0; return block; } #endif - - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) + block = kmalloc(EDID_LENGTH, GFP_KERNEL); + if (block == NULL) { + result = EDID_ERR_NO_MEM; goto error; + }
/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) goto error_free; - if (drm_edid_block_valid(block, 0, print_bad_edid)) + result = drm_edid_block_check_error(block, 0, last_error_flags); + if (!result) break; - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { - connector->null_edid_counter++; + if (i == 0 && result & EDID_ERR_NULL) goto error_carp; - } } if (i == 4) goto error_carp;
/* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) - return block; + goto done;
/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */ if (block[EDID_VERSION_MINOR_OFFSET] < 3) @@ -494,7 +513,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
/* see if EDID is in the cache - no need to read all extension blocks */ if (compare_get_edid_from_cache(connector, (struct edid **)&block)) - return block; + goto done;
new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) { @@ -512,7 +531,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) valid_extensions = 0; goto done_fix_extension_count; } - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { + if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) { valid_extensions++; /* Test if base block announced extension blocks although * display is not EDDC capable. @@ -538,21 +557,22 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
done_fix_extension_count: fixup_edid(&block, valid_extensions); +done: drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL); + connector->last_edid_error_flags = 0;
return block;
error_carp: - if (print_bad_edid) { + if (last_error_flags != result) { dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } - connector->bad_edid_counter++; - error_free: kfree(block); error: drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = result; return NULL; }
@@ -569,17 +589,19 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) { int n_blocks = len / EDID_LENGTH; - int valid_extensions = 0, ret = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int valid_extensions = 0, ret = -EINVAL; + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
- if (!blockp || !*blockp) - ret = -EINVAL; - else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) { - kfree(*blockp); - *blockp = NULL; - ret = -EINVAL; + if (blockp && *blockp) { + if (n_blocks) + result = drm_edid_block_check_error(*blockp, 0, last_error_flags); + if (result) { + kfree(*blockp); + *blockp = NULL; + } } - if (!ret) { + if (!result) { int cnt = 0; n_blocks--; if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks) @@ -587,8 +609,8 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len)
while (n_blocks--) { cnt++; - if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH, - valid_extensions + 1, print_bad_edid)) { + if (!drm_edid_block_check_error(*blockp + cnt * EDID_LENGTH, + valid_extensions + 1, last_error_flags)) { valid_extensions++; if (cnt != valid_extensions) memcpy(*blockp + valid_extensions * EDID_LENGTH, @@ -596,8 +618,9 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) } } fixup_edid(blockp, valid_extensions); - } else - connector->bad_edid_counter++; + ret = 0; + } + connector->last_edid_error_flags = result; return ret; }
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index b884c36..e80ba63 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -961,7 +961,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) drm_get_connector_name(connector)); /* rs690 seems to have a problem with connectors not existing and always * return a block of 0's. If we see this just stop polling on this output */ - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) { + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.last_edid_error_flags & EDID_ERR_NULL) { ret = connector_status_disconnected; DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector)); radeon_connector->ddc_bus = NULL; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6a1054c..d402b3b 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -608,8 +608,7 @@ struct drm_connector { bool latency_present[2]; int video_latency[2]; /* [0]: progressive, [1]: interlaced */ int audio_latency[2]; - int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */ - unsigned bad_edid_counter; + unsigned last_edid_error_flags; };
/** @@ -1056,7 +1055,6 @@ extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); extern bool drm_edid_is_valid(struct edid *edid); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c880510..6bcaee5 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -244,6 +244,15 @@ struct edid {
#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
+enum edid_error { + EDID_ERR_NO_BLOCK0 = 1 << 0, + EDID_ERR_NULL = 1 << 1, + EDID_ERR_CSUM = 1 << 2, + EDID_ERR_UNSUPPORTED_VERSION = 1 << 3, + EDID_ERR_NO_DATA = 1 << 4, + EDID_ERR_NO_MEM = 1 << 5, +}; + struct drm_encoder; struct drm_connector; struct drm_display_mode; @@ -257,5 +266,6 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); void drm_cache_edid(struct drm_connector *connector, struct edid *edid); +unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
#endif /* __DRM_EDID_H__ */
Consolidate the null_edid_counter and the bad_edid_counter into EDID error state flags which for the last EDID read are accessible from user. Errors are looged it the same error has not been present in the previous read of the EDID. This will reset the EDID error status for example when the monitor is changed but still prevents permanent EDID errors from piling up the the kernel logs.
v2: Fixed conflits due to reordering of commits. Set error state where missing. v3: Don't update cache when returning block from cache.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 117 +++++++++++++++++----------- drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 4 +- include/drm/drm_edid.h | 10 +++ 4 files changed, 82 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index dd0df60..aa9b34d 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid) } EXPORT_SYMBOL(drm_edid_header_is_valid);
+static bool drm_edid_is_zero(u8 *in_edid, int length) +{ + int i; + u32 *raw_edid = (u32 *)in_edid; + + for (i = 0; i < length / 4; i++) + if (*(raw_edid + i) != 0) + return false; + return true; +} + static int edid_fixup __read_mostly = 6; module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, @@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup, * Sanity check the EDID block (base or extension). Return 0 if the block * doesn't check out, or 1 if it's valid. */ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) +unsigned +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags) { int i; u8 csum = 0; struct edid *edid = (struct edid *)raw_edid; + unsigned result = 0;
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6; @@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); memcpy(raw_edid, edid_header, sizeof(edid_header)); } else { - goto bad; + result |= EDID_ERR_NO_BLOCK0; + if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) { + result |= EDID_ERR_NULL; + goto bad; + } } }
for (i = 0; i < EDID_LENGTH; i++) csum += raw_edid[i]; if (csum) { - if (print_bad_edid) { + if ((last_error_flags & EDID_ERR_CSUM) == 0) DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum); - }
/* allow CEA to slide through, switches mangle this */ if (raw_edid[0] != 0x02) - goto bad; + result |= EDID_ERR_CSUM; } + if (result) + goto bad;
/* per-block-type checks */ switch (raw_edid[0]) { case 0: /* base */ if (edid->version != 1) { DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version); + result |= EDID_ERR_UNSUPPORTED_VERSION; goto bad; }
@@ -214,15 +233,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1; + return 0;
bad: - if (raw_edid && print_bad_edid) { + if (raw_edid && last_error_flags != result) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); } - return 0; + return result; +} + +bool +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags) +{ + if (!drm_edid_block_check_error(raw_edid, block, last_error_flags)) + return true; + return false; } EXPORT_SYMBOL(drm_edid_block_valid);
@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid) return false;
for (i = 0; i <= edid->extensions; i++) - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true)) + if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true)) return false;
return true; @@ -310,17 +337,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, return ret == xfers ? 0 : -1; }
-static bool drm_edid_is_zero(u8 *in_edid, int length) -{ - int i; - u32 *raw_edid = (u32 *)in_edid; - - for (i = 0; i < length / 4; i++) - if (*(raw_edid + i) != 0) - return false; - return true; -} - static void fix_map(u8 *block, int cnt) { @@ -456,37 +472,40 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { int i, j = 0, valid_extensions = 0; u8 *block, *new; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector); if (block) { drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = 0; return block; } #endif - - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) + block = kmalloc(EDID_LENGTH, GFP_KERNEL); + if (block == NULL) { + result = EDID_ERR_NO_MEM; goto error; + }
/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) goto error_free; - if (drm_edid_block_valid(block, 0, print_bad_edid)) + result = drm_edid_block_check_error(block, 0, last_error_flags); + if (!result) break; - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { - connector->null_edid_counter++; + if (i == 0 && result & EDID_ERR_NULL) goto error_carp; - } } if (i == 4) goto error_carp;
/* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) - goto done; + goto done_update_cache;
/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */ if (block[EDID_VERSION_MINOR_OFFSET] < 3) @@ -494,7 +513,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
/* see if EDID is in the cache - no need to read all extension blocks */ if (compare_get_edid_from_cache(connector, (struct edid **)&block)) - return block; + goto done;
new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) { @@ -512,7 +531,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) valid_extensions = 0; goto done_fix_extension_count; } - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { + if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) { valid_extensions++; /* If extension block 2 is identical to the base block the display is probably * not EDDC cabable - despite of what the extension flag says - as it doesn't @@ -535,22 +554,23 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
done_fix_extension_count: fixup_edid(&block, valid_extensions); -done: +done_update_cache: drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL); +done: + connector->last_edid_error_flags = 0;
return block;
error_carp: - if (print_bad_edid) { + if (last_error_flags != result) { dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } - connector->bad_edid_counter++; - error_free: kfree(block); error: drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = result; return NULL; }
@@ -567,17 +587,19 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) { int n_blocks = len / EDID_LENGTH; - int valid_extensions = 0, ret = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int valid_extensions = 0, ret = -EINVAL; + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
- if (!blockp || !*blockp) - ret = -EINVAL; - else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) { - kfree(*blockp); - *blockp = NULL; - ret = -EINVAL; + if (blockp && *blockp) { + if (n_blocks) + result = drm_edid_block_check_error(*blockp, 0, last_error_flags); + if (result) { + kfree(*blockp); + *blockp = NULL; + } } - if (!ret) { + if (!result) { int cnt = 0; n_blocks--; if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks) @@ -585,8 +607,8 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len)
while (n_blocks--) { cnt++; - if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH, - valid_extensions + 1, print_bad_edid)) { + if (!drm_edid_block_check_error(*blockp + cnt * EDID_LENGTH, + valid_extensions + 1, last_error_flags)) { valid_extensions++; if (cnt != valid_extensions) memcpy(*blockp + valid_extensions * EDID_LENGTH, @@ -594,8 +616,9 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) } } fixup_edid(blockp, valid_extensions); - } else - connector->bad_edid_counter++; + ret = 0; + } + connector->last_edid_error_flags = result; return ret; }
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index b884c36..e80ba63 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -961,7 +961,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) drm_get_connector_name(connector)); /* rs690 seems to have a problem with connectors not existing and always * return a block of 0's. If we see this just stop polling on this output */ - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) { + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.last_edid_error_flags & EDID_ERR_NULL) { ret = connector_status_disconnected; DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector)); radeon_connector->ddc_bus = NULL; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6a1054c..d402b3b 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -608,8 +608,7 @@ struct drm_connector { bool latency_present[2]; int video_latency[2]; /* [0]: progressive, [1]: interlaced */ int audio_latency[2]; - int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */ - unsigned bad_edid_counter; + unsigned last_edid_error_flags; };
/** @@ -1056,7 +1055,6 @@ extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); extern bool drm_edid_is_valid(struct edid *edid); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c880510..6bcaee5 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -244,6 +244,15 @@ struct edid {
#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
+enum edid_error { + EDID_ERR_NO_BLOCK0 = 1 << 0, + EDID_ERR_NULL = 1 << 1, + EDID_ERR_CSUM = 1 << 2, + EDID_ERR_UNSUPPORTED_VERSION = 1 << 3, + EDID_ERR_NO_DATA = 1 << 4, + EDID_ERR_NO_MEM = 1 << 5, +}; + struct drm_encoder; struct drm_connector; struct drm_display_mode; @@ -257,5 +266,6 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); void drm_cache_edid(struct drm_connector *connector, struct edid *edid); +unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
#endif /* __DRM_EDID_H__ */
On Thu, Nov 22, 2012 at 09:44:42AM -0500, Egbert Eich wrote:
Consolidate the null_edid_counter and the bad_edid_counter into EDID error state flags which for the last EDID read are accessible from user. Errors are looged it the same error has not been present in the previous read of the EDID. This will reset the EDID error status for example when the monitor is changed but still prevents permanent EDID errors from piling up the the kernel logs.
v2: Fixed conflits due to reordering of commits. Set error state where missing. v3: Don't update cache when returning block from cache.
Signed-off-by: Egbert Eich eich@suse.com
drivers/gpu/drm/drm_edid.c | 117 +++++++++++++++++----------- drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 4 +- include/drm/drm_edid.h | 10 +++ 4 files changed, 82 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index dd0df60..aa9b34d 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid) } EXPORT_SYMBOL(drm_edid_header_is_valid);
+static bool drm_edid_is_zero(u8 *in_edid, int length) +{
- int i;
- u32 *raw_edid = (u32 *)in_edid;
- for (i = 0; i < length / 4; i++)
if (*(raw_edid + i) != 0)return false;- return true;
You could use memchr_inv() here. But the compiler can't optimize it since it's not inline, so I suppose it might make it slower.
+}
static int edid_fixup __read_mostly = 6; module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, @@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup,
- Sanity check the EDID block (base or extension). Return 0 if the block
- doesn't check out, or 1 if it's valid.
*/ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) +unsigned +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags) { int i; u8 csum = 0; struct edid *edid = (struct edid *)raw_edid;
unsigned result = 0;
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6;
@@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); memcpy(raw_edid, edid_header, sizeof(edid_header)); } else {
goto bad;
result |= EDID_ERR_NO_BLOCK0;if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {result |= EDID_ERR_NULL;goto bad;}} }
for (i = 0; i < EDID_LENGTH; i++) csum += raw_edid[i]; if (csum) {
if (print_bad_edid) {
if ((last_error_flags & EDID_ERR_CSUM) == 0) DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum);
}/* allow CEA to slide through, switches mangle this */ if (raw_edid[0] != 0x02)
goto bad;
result |= EDID_ERR_CSUM;}
if (result)
goto bad;/* per-block-type checks */ switch (raw_edid[0]) { case 0: /* base */ if (edid->version != 1) { DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version);
result |= EDID_ERR_UNSUPPORTED_VERSION; goto bad;}
@@ -214,15 +233,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1;
- return 0;
bad:
- if (raw_edid && print_bad_edid) {
- if (raw_edid && last_error_flags != result) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); }
- return 0;
- return result;
+}
+bool +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags) +{
- if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
return true;- return false;
return !drm_edid_block_check_error();
} EXPORT_SYMBOL(drm_edid_block_valid);
@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid) return false;
for (i = 0; i <= edid->extensions; i++)
if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
^^^^
That looks wrong. Also the 's/!drm_edid_block_valid/drm_edid_block_check_error' change seems superfluous since you're not using the more detailed return value from drm_edid_block_check_error().
return false;return true; @@ -310,17 +337,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, return ret == xfers ? 0 : -1; }
-static bool drm_edid_is_zero(u8 *in_edid, int length) -{
- int i;
- u32 *raw_edid = (u32 *)in_edid;
- for (i = 0; i < length / 4; i++)
if (*(raw_edid + i) != 0)return false;- return true;
-}
static void fix_map(u8 *block, int cnt) { @@ -456,37 +472,40 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { int i, j = 0, valid_extensions = 0; u8 *block, *new;
- bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
- int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags;
- unsigned result = EDID_ERR_NO_DATA;
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector); if (block) { drm_cache_edid(connector, NULL);
connector->last_edid_error_flags = 0;#endif
- if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
block = kmalloc(EDID_LENGTH, GFP_KERNEL);
if (block == NULL) {
result = EDID_ERR_NO_MEM;goto error;
}
/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) goto error_free;
if (drm_edid_block_valid(block, 0, print_bad_edid))
result = drm_edid_block_check_error(block, 0, last_error_flags);if (!result) break;
if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {connector->null_edid_counter++;
if (i == 0 && result & EDID_ERR_NULL) goto error_carp;
}} if (i == 4) goto error_carp;
/* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
goto done;
goto done_update_cache;/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */ if (block[EDID_VERSION_MINOR_OFFSET] < 3)
@@ -494,7 +513,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
/* see if EDID is in the cache - no need to read all extension blocks */ if (compare_get_edid_from_cache(connector, (struct edid **)&block))
return block;
goto done;new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) {
@@ -512,7 +531,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) valid_extensions = 0; goto done_fix_extension_count; }
if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
if (!drm_edid_block_check_error(block + (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {
Again the change of function seems superfluous.
Consolidate the null_edid_counter and the bad_edid_counter into EDID error state flags which for the last EDID read are accessible from user. Errors are looged it the same error has not been present in the previous read of the EDID. This will reset the EDID error status for example when the monitor is changed but still prevents permanent EDID errors from piling up the the kernel logs.
v2: Fixed conflits due to reordering of commits. Set error state where missing. v3: Don't update cache when returning block from cache. v4: Fixed drm_edid_is_valid() to return the actual error state. Included results of extension block checking in the overall error state. Removed drm_edid_block_valid() as there is noone using it.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 136 ++++++++++++++++------------ drivers/gpu/drm/radeon/radeon_atombios.c | 2 +- drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 6 +- include/drm/drm_edid.h | 10 ++ 5 files changed, 94 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index dd0df60..6cedd46 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid) } EXPORT_SYMBOL(drm_edid_header_is_valid);
+static bool drm_edid_is_zero(u8 *in_edid, int length) +{ + int i; + u32 *raw_edid = (u32 *)in_edid; + + for (i = 0; i < length / 4; i++) + if (*(raw_edid + i) != 0) + return false; + return true; +} + static int edid_fixup __read_mostly = 6; module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, @@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup, * Sanity check the EDID block (base or extension). Return 0 if the block * doesn't check out, or 1 if it's valid. */ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) +unsigned +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags) { int i; u8 csum = 0; struct edid *edid = (struct edid *)raw_edid; + unsigned result = 0;
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6; @@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); memcpy(raw_edid, edid_header, sizeof(edid_header)); } else { - goto bad; + result |= EDID_ERR_NO_BLOCK0; + if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) { + result |= EDID_ERR_NULL; + goto bad; + } } }
for (i = 0; i < EDID_LENGTH; i++) csum += raw_edid[i]; if (csum) { - if (print_bad_edid) { + if ((last_error_flags & EDID_ERR_CSUM) == 0) DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum); - }
/* allow CEA to slide through, switches mangle this */ if (raw_edid[0] != 0x02) - goto bad; + result |= EDID_ERR_CSUM; } + if (result) + goto bad;
/* per-block-type checks */ switch (raw_edid[0]) { case 0: /* base */ if (edid->version != 1) { DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version); + result |= EDID_ERR_UNSUPPORTED_VERSION; goto bad; }
@@ -214,17 +233,16 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1; + return 0;
bad: - if (raw_edid && print_bad_edid) { + if (raw_edid && last_error_flags != result) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); } - return 0; + return result; } -EXPORT_SYMBOL(drm_edid_block_valid);
/** * drm_edid_is_valid - sanity check EDID data @@ -232,19 +250,21 @@ EXPORT_SYMBOL(drm_edid_block_valid); * * Sanity-check an entire EDID record (including extensions) */ -bool drm_edid_is_valid(struct edid *edid) +unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags) { int i; + unsigned result; u8 *raw = (u8 *)edid;
if (!edid) - return false; + return EDID_ERR_NO_DATA;
- for (i = 0; i <= edid->extensions; i++) - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true)) - return false; - - return true; + for (i = 0; i <= edid->extensions; i++) { + result = drm_edid_block_check_error(raw + i * EDID_LENGTH, i, last_error_flags); + if (result) + return result; + } + return 0; } EXPORT_SYMBOL(drm_edid_is_valid);
@@ -310,17 +330,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, return ret == xfers ? 0 : -1; }
-static bool drm_edid_is_zero(u8 *in_edid, int length) -{ - int i; - u32 *raw_edid = (u32 *)in_edid; - - for (i = 0; i < length / 4; i++) - if (*(raw_edid + i) != 0) - return false; - return true; -} - static void fix_map(u8 *block, int cnt) { @@ -456,37 +465,40 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { int i, j = 0, valid_extensions = 0; u8 *block, *new; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector); if (block) { drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = 0; return block; } #endif - - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) + block = kmalloc(EDID_LENGTH, GFP_KERNEL); + if (block == NULL) { + result = EDID_ERR_NO_MEM; goto error; + }
/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) goto error_free; - if (drm_edid_block_valid(block, 0, print_bad_edid)) + result = drm_edid_block_check_error(block, 0, last_error_flags); + if (!result) break; - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { - connector->null_edid_counter++; + if (i == 0 && result & EDID_ERR_NULL) goto error_carp; - } } - if (i == 4) + if (result) goto error_carp;
/* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) - goto done; + goto done_update_cache;
/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */ if (block[EDID_VERSION_MINOR_OFFSET] < 3) @@ -494,7 +506,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
/* see if EDID is in the cache - no need to read all extension blocks */ if (compare_get_edid_from_cache(connector, (struct edid **)&block)) - return block; + goto done;
new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) { @@ -506,13 +518,18 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) { for (i = 0; i < 4; i++) { + unsigned err; if (drm_do_probe_ddc_edid(adapter, block + (valid_extensions + 1) * EDID_LENGTH, j, EDID_LENGTH)) { valid_extensions = 0; goto done_fix_extension_count; } - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { + err = drm_edid_block_check_error( + block + (valid_extensions + 1) * EDID_LENGTH, + j, last_error_flags); + result |= err; + if (!err) { valid_extensions++; /* If extension block 2 is identical to the base block the display is probably * not EDDC cabable - despite of what the extension flag says - as it doesn't @@ -535,22 +552,23 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
done_fix_extension_count: fixup_edid(&block, valid_extensions); -done: +done_update_cache: drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL); +done: + connector->last_edid_error_flags = 0;
return block;
error_carp: - if (print_bad_edid) { + if (last_error_flags != result) { dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } - connector->bad_edid_counter++; - error_free: kfree(block); error: drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = result; return NULL; }
@@ -567,26 +585,31 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) { int n_blocks = len / EDID_LENGTH; - int valid_extensions = 0, ret = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int valid_extensions = 0, ret = -EINVAL; + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
- if (!blockp || !*blockp) - ret = -EINVAL; - else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) { - kfree(*blockp); - *blockp = NULL; - ret = -EINVAL; + if (blockp && *blockp) { + if (n_blocks) + result = drm_edid_block_check_error(*blockp, 0, last_error_flags); + if (result) { + kfree(*blockp); + *blockp = NULL; + } } - if (!ret) { + if (!result) { int cnt = 0; n_blocks--; if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks) n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
while (n_blocks--) { + unsigned err; cnt++; - if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH, - valid_extensions + 1, print_bad_edid)) { + err = drm_edid_block_check_error(*blockp + cnt * EDID_LENGTH, + valid_extensions + 1, last_error_flags); + result |= err; + if (!err) { valid_extensions++; if (cnt != valid_extensions) memcpy(*blockp + valid_extensions * EDID_LENGTH, @@ -594,8 +617,9 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) } } fixup_edid(blockp, valid_extensions); - } else - connector->bad_edid_counter++; + ret = 0; + } + connector->last_edid_error_flags = result; return ret; }
@@ -2187,7 +2211,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) if (edid == NULL) { return 0; } - if (!drm_edid_is_valid(edid)) { + if (drm_edid_is_valid(edid, 0)) { dev_warn(connector->dev->dev, "%s: EDID invalid.\n", drm_get_connector_name(connector)); return 0; diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c index f22eb57..2de0d5b 100644 --- a/drivers/gpu/drm/radeon/radeon_atombios.c +++ b/drivers/gpu/drm/radeon/radeon_atombios.c @@ -1644,7 +1644,7 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(struct memcpy((u8 *)edid, (u8 *)&fake_edid_record->ucFakeEDIDString[0], fake_edid_record->ucFakeEDIDLength);
- if (drm_edid_is_valid(edid)) { + if (drm_edid_is_valid(edid,0) == 0) { rdev->mode_info.bios_hardcoded_edid = edid; rdev->mode_info.bios_hardcoded_edid_size = edid_size; } else diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index b884c36..e80ba63 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -961,7 +961,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) drm_get_connector_name(connector)); /* rs690 seems to have a problem with connectors not existing and always * return a block of 0's. If we see this just stop polling on this output */ - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) { + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.last_edid_error_flags & EDID_ERR_NULL) { ret = connector_status_disconnected; DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector)); radeon_connector->ddc_bus = NULL; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6a1054c..7a3ccbf 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -608,8 +608,7 @@ struct drm_connector { bool latency_present[2]; int video_latency[2]; /* [0]: progressive, [1]: interlaced */ int audio_latency[2]; - int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */ - unsigned bad_edid_counter; + unsigned last_edid_error_flags; };
/** @@ -1056,8 +1055,7 @@ extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); -extern bool drm_edid_is_valid(struct edid *edid); +extern unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, bool rb); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c880510..6bcaee5 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -244,6 +244,15 @@ struct edid {
#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
+enum edid_error { + EDID_ERR_NO_BLOCK0 = 1 << 0, + EDID_ERR_NULL = 1 << 1, + EDID_ERR_CSUM = 1 << 2, + EDID_ERR_UNSUPPORTED_VERSION = 1 << 3, + EDID_ERR_NO_DATA = 1 << 4, + EDID_ERR_NO_MEM = 1 << 5, +}; + struct drm_encoder; struct drm_connector; struct drm_display_mode; @@ -257,5 +266,6 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); void drm_cache_edid(struct drm_connector *connector, struct edid *edid); +unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
#endif /* __DRM_EDID_H__ */
Consolidate the null_edid_counter and the bad_edid_counter into EDID error state flags which for the last EDID read are accessible from user. Errors are looged it the same error has not been present in the previous read of the EDID. This will reset the EDID error status for example when the monitor is changed but still prevents permanent EDID errors from piling up the the kernel logs.
v2: Fixed conflits due to reordering of commits. Set error state where missing. v3: Don't update cache when returning block from cache. v4: Inspired by a discussion with Ville Syrjälä Fixed drm_edid_is_valid() to return the actual error state. Included results of extension block checking in the overall error state. Removed drm_edid_block_valid() as there is noone using it. v5: Added missing radeon/radeon_combios.c.
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/drm_edid.c | 136 ++++++++++++++++------------ drivers/gpu/drm/radeon/radeon_atombios.c | 2 +- drivers/gpu/drm/radeon/radeon_combios.c | 2 +- drivers/gpu/drm/radeon/radeon_connectors.c | 2 +- include/drm/drm_crtc.h | 6 +- include/drm/drm_edid.h | 10 ++ 6 files changed, 95 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index dd0df60..6cedd46 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid) } EXPORT_SYMBOL(drm_edid_header_is_valid);
+static bool drm_edid_is_zero(u8 *in_edid, int length) +{ + int i; + u32 *raw_edid = (u32 *)in_edid; + + for (i = 0; i < length / 4; i++) + if (*(raw_edid + i) != 0) + return false; + return true; +} + static int edid_fixup __read_mostly = 6; module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, @@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup, * Sanity check the EDID block (base or extension). Return 0 if the block * doesn't check out, or 1 if it's valid. */ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) +unsigned +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags) { int i; u8 csum = 0; struct edid *edid = (struct edid *)raw_edid; + unsigned result = 0;
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6; @@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); memcpy(raw_edid, edid_header, sizeof(edid_header)); } else { - goto bad; + result |= EDID_ERR_NO_BLOCK0; + if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) { + result |= EDID_ERR_NULL; + goto bad; + } } }
for (i = 0; i < EDID_LENGTH; i++) csum += raw_edid[i]; if (csum) { - if (print_bad_edid) { + if ((last_error_flags & EDID_ERR_CSUM) == 0) DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum); - }
/* allow CEA to slide through, switches mangle this */ if (raw_edid[0] != 0x02) - goto bad; + result |= EDID_ERR_CSUM; } + if (result) + goto bad;
/* per-block-type checks */ switch (raw_edid[0]) { case 0: /* base */ if (edid->version != 1) { DRM_ERROR("EDID has major version %d, instead of 1\n", edid->version); + result |= EDID_ERR_UNSUPPORTED_VERSION; goto bad; }
@@ -214,17 +233,16 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1; + return 0;
bad: - if (raw_edid && print_bad_edid) { + if (raw_edid && last_error_flags != result) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); } - return 0; + return result; } -EXPORT_SYMBOL(drm_edid_block_valid);
/** * drm_edid_is_valid - sanity check EDID data @@ -232,19 +250,21 @@ EXPORT_SYMBOL(drm_edid_block_valid); * * Sanity-check an entire EDID record (including extensions) */ -bool drm_edid_is_valid(struct edid *edid) +unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags) { int i; + unsigned result; u8 *raw = (u8 *)edid;
if (!edid) - return false; + return EDID_ERR_NO_DATA;
- for (i = 0; i <= edid->extensions; i++) - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true)) - return false; - - return true; + for (i = 0; i <= edid->extensions; i++) { + result = drm_edid_block_check_error(raw + i * EDID_LENGTH, i, last_error_flags); + if (result) + return result; + } + return 0; } EXPORT_SYMBOL(drm_edid_is_valid);
@@ -310,17 +330,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, return ret == xfers ? 0 : -1; }
-static bool drm_edid_is_zero(u8 *in_edid, int length) -{ - int i; - u32 *raw_edid = (u32 *)in_edid; - - for (i = 0; i < length / 4; i++) - if (*(raw_edid + i) != 0) - return false; - return true; -} - static void fix_map(u8 *block, int cnt) { @@ -456,37 +465,40 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { int i, j = 0, valid_extensions = 0; u8 *block, *new; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE /* check if the user has specified a 'firmware' EDID file */ block = (u8 *)drm_load_edid_firmware(connector); if (block) { drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = 0; return block; } #endif - - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) + block = kmalloc(EDID_LENGTH, GFP_KERNEL); + if (block == NULL) { + result = EDID_ERR_NO_MEM; goto error; + }
/* base block fetch */ for (i = 0; i < 4; i++) { if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH)) goto error_free; - if (drm_edid_block_valid(block, 0, print_bad_edid)) + result = drm_edid_block_check_error(block, 0, last_error_flags); + if (!result) break; - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { - connector->null_edid_counter++; + if (i == 0 && result & EDID_ERR_NULL) goto error_carp; - } } - if (i == 4) + if (result) goto error_carp;
/* if there are no extensions, we're done - don't bother caching */ if (block[EDID_EXTENSION_FLAG_OFFSET] == 0) - goto done; + goto done_update_cache;
/* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */ if (block[EDID_VERSION_MINOR_OFFSET] < 3) @@ -494,7 +506,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
/* see if EDID is in the cache - no need to read all extension blocks */ if (compare_get_edid_from_cache(connector, (struct edid **)&block)) - return block; + goto done;
new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) { @@ -506,13 +518,18 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
for (j = 1; j <= block[EDID_EXTENSION_FLAG_OFFSET]; j++) { for (i = 0; i < 4; i++) { + unsigned err; if (drm_do_probe_ddc_edid(adapter, block + (valid_extensions + 1) * EDID_LENGTH, j, EDID_LENGTH)) { valid_extensions = 0; goto done_fix_extension_count; } - if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) { + err = drm_edid_block_check_error( + block + (valid_extensions + 1) * EDID_LENGTH, + j, last_error_flags); + result |= err; + if (!err) { valid_extensions++; /* If extension block 2 is identical to the base block the display is probably * not EDDC cabable - despite of what the extension flag says - as it doesn't @@ -535,22 +552,23 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
done_fix_extension_count: fixup_edid(&block, valid_extensions); -done: +done_update_cache: drm_cache_edid(connector, (valid_extensions > 0) ? (struct edid *)block : NULL); +done: + connector->last_edid_error_flags = 0;
return block;
error_carp: - if (print_bad_edid) { + if (last_error_flags != result) { dev_warn(connector->dev->dev, "%s: EDID block %d invalid.\n", drm_get_connector_name(connector), j); } - connector->bad_edid_counter++; - error_free: kfree(block); error: drm_cache_edid(connector, NULL); + connector->last_edid_error_flags = result; return NULL; }
@@ -567,26 +585,31 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) { int n_blocks = len / EDID_LENGTH; - int valid_extensions = 0, ret = 0; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + int valid_extensions = 0, ret = -EINVAL; + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags; + unsigned result = EDID_ERR_NO_DATA;
- if (!blockp || !*blockp) - ret = -EINVAL; - else if (!n_blocks || !drm_edid_block_valid(*blockp, 0, print_bad_edid)) { - kfree(*blockp); - *blockp = NULL; - ret = -EINVAL; + if (blockp && *blockp) { + if (n_blocks) + result = drm_edid_block_check_error(*blockp, 0, last_error_flags); + if (result) { + kfree(*blockp); + *blockp = NULL; + } } - if (!ret) { + if (!result) { int cnt = 0; n_blocks--; if ((*blockp)[EDID_EXTENSION_FLAG_OFFSET] < n_blocks) n_blocks = (*blockp)[EDID_EXTENSION_FLAG_OFFSET];
while (n_blocks--) { + unsigned err; cnt++; - if (drm_edid_block_valid(*blockp + cnt * EDID_LENGTH, - valid_extensions + 1, print_bad_edid)) { + err = drm_edid_block_check_error(*blockp + cnt * EDID_LENGTH, + valid_extensions + 1, last_error_flags); + result |= err; + if (!err) { valid_extensions++; if (cnt != valid_extensions) memcpy(*blockp + valid_extensions * EDID_LENGTH, @@ -594,8 +617,9 @@ drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len) } } fixup_edid(blockp, valid_extensions); - } else - connector->bad_edid_counter++; + ret = 0; + } + connector->last_edid_error_flags = result; return ret; }
@@ -2187,7 +2211,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) if (edid == NULL) { return 0; } - if (!drm_edid_is_valid(edid)) { + if (drm_edid_is_valid(edid, 0)) { dev_warn(connector->dev->dev, "%s: EDID invalid.\n", drm_get_connector_name(connector)); return 0; diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c index f22eb57..2de0d5b 100644 --- a/drivers/gpu/drm/radeon/radeon_atombios.c +++ b/drivers/gpu/drm/radeon/radeon_atombios.c @@ -1644,7 +1644,7 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(struct memcpy((u8 *)edid, (u8 *)&fake_edid_record->ucFakeEDIDString[0], fake_edid_record->ucFakeEDIDLength);
- if (drm_edid_is_valid(edid)) { + if (drm_edid_is_valid(edid,0) == 0) { rdev->mode_info.bios_hardcoded_edid = edid; rdev->mode_info.bios_hardcoded_edid_size = edid_size; } else diff --git a/drivers/gpu/drm/radeon/radeon_combios.c b/drivers/gpu/drm/radeon/radeon_combios.c index 45b660b..42cf702 100644 --- a/drivers/gpu/drm/radeon/radeon_combios.c +++ b/drivers/gpu/drm/radeon/radeon_combios.c @@ -463,7 +463,7 @@ bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev)
memcpy((unsigned char *)edid, raw, size);
- if (!drm_edid_is_valid(edid)) { + if (drm_edid_is_valid(edid,0) != 0) { kfree(edid); return false; } diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index b884c36..e80ba63 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -961,7 +961,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) drm_get_connector_name(connector)); /* rs690 seems to have a problem with connectors not existing and always * return a block of 0's. If we see this just stop polling on this output */ - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) { + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.last_edid_error_flags & EDID_ERR_NULL) { ret = connector_status_disconnected; DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector)); radeon_connector->ddc_bus = NULL; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6a1054c..7a3ccbf 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -608,8 +608,7 @@ struct drm_connector { bool latency_present[2]; int video_latency[2]; /* [0]: progressive, [1]: interlaced */ int audio_latency[2]; - int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */ - unsigned bad_edid_counter; + unsigned last_edid_error_flags; };
/** @@ -1056,8 +1055,7 @@ extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid); -extern bool drm_edid_is_valid(struct edid *edid); +extern unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, bool rb); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c880510..6bcaee5 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -244,6 +244,15 @@ struct edid {
#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
+enum edid_error { + EDID_ERR_NO_BLOCK0 = 1 << 0, + EDID_ERR_NULL = 1 << 1, + EDID_ERR_CSUM = 1 << 2, + EDID_ERR_UNSUPPORTED_VERSION = 1 << 3, + EDID_ERR_NO_DATA = 1 << 4, + EDID_ERR_NO_MEM = 1 << 5, +}; + struct drm_encoder; struct drm_connector; struct drm_display_mode; @@ -257,5 +266,6 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector); #endif int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len); void drm_cache_edid(struct drm_connector *connector, struct edid *edid); +unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
#endif /* __DRM_EDID_H__ */
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/ast/ast_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 7fc9f72..c27aa8d 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -31,6 +31,7 @@ #include <drm/drmP.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h> #include "ast_drv.h"
#include "ast_tables.h"
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/gma500/cdv_intel_dp.c | 1 + drivers/gpu/drm/gma500/oaktrail_lvds.c | 1 + drivers/gpu/drm/gma500/psb_intel_modes.c | 1 + 3 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c index e3a3978..37cad73 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_dp.c +++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c @@ -34,6 +34,7 @@ #include "psb_intel_drv.h" #include "psb_intel_reg.h" #include <drm/drm_dp_helper.h> +#include <drm/drm_edid.h>
#define _wait_for(COND, MS, W) ({ \ unsigned long timeout__ = jiffies + msecs_to_jiffies(MS); \ diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c index 558c77f..f7e20f5 100644 --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c @@ -22,6 +22,7 @@
#include <linux/i2c.h> #include <drm/drmP.h> +#include <drm/drm_edid.h> #include <asm/mrst.h>
#include "intel_bios.h" diff --git a/drivers/gpu/drm/gma500/psb_intel_modes.c b/drivers/gpu/drm/gma500/psb_intel_modes.c index 4fca0d6..1da014d 100644 --- a/drivers/gpu/drm/gma500/psb_intel_modes.c +++ b/drivers/gpu/drm/gma500/psb_intel_modes.c @@ -20,6 +20,7 @@ #include <linux/i2c.h> #include <linux/fb.h> #include <drm/drmP.h> +#include <drm/drm_edid.h> #include "psb_intel_drv.h"
/**
Signed-off-by: Egbert Eich eich@suse.com --- drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index d3d99a2..f89a0c1 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -15,6 +15,7 @@
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h>
#include "mgag200_drv.h"
Signed-off-by: Egbert Eich eich@suse.com --- include/drm/drm_crtc.h | 8 -------- include/drm/drm_edid.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d402b3b..7eed9bd 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -878,9 +878,6 @@ extern char *drm_get_tv_subconnector_name(int val); extern char *drm_get_tv_select_name(int val); extern void drm_fb_release(struct drm_file *file_priv); extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group); -extern bool drm_probe_ddc(struct i2c_adapter *adapter); -extern struct edid *drm_get_edid(struct drm_connector *connector, - struct i2c_adapter *adapter); extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid); extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode); extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode); @@ -1036,9 +1033,6 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern int drm_mode_gamma_set_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -extern u8 *drm_find_cea_extension(struct edid *edid); -extern bool drm_detect_hdmi_monitor(struct edid *edid); -extern bool drm_detect_monitor_audio(struct edid *edid); extern int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, @@ -1054,8 +1048,6 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev, extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
-extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern bool drm_edid_is_valid(struct edid *edid); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, bool rb); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 6bcaee5..72328bc 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -268,4 +268,13 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len void drm_cache_edid(struct drm_connector *connector, struct edid *edid); unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
+extern bool drm_probe_ddc(struct i2c_adapter *adapter); +extern struct edid *drm_get_edid(struct drm_connector *connector, + struct i2c_adapter *adapter); +extern u8 *drm_find_cea_extension(struct edid *edid); +extern bool drm_detect_hdmi_monitor(struct edid *edid); +extern bool drm_detect_monitor_audio(struct edid *edid); +extern int drm_edid_header_is_valid(const u8 *raw_edid); +extern bool drm_edid_is_valid(struct edid *edid); + #endif /* __DRM_EDID_H__ */
v2: Adjusted to apply cleanly.
Signed-off-by: Egbert Eich eich@suse.com --- include/drm/drm_crtc.h | 8 -------- include/drm/drm_edid.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 7a3ccbf..7eed9bd 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -878,9 +878,6 @@ extern char *drm_get_tv_subconnector_name(int val); extern char *drm_get_tv_select_name(int val); extern void drm_fb_release(struct drm_file *file_priv); extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group); -extern bool drm_probe_ddc(struct i2c_adapter *adapter); -extern struct edid *drm_get_edid(struct drm_connector *connector, - struct i2c_adapter *adapter); extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid); extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode); extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode); @@ -1036,9 +1033,6 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern int drm_mode_gamma_set_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -extern u8 *drm_find_cea_extension(struct edid *edid); -extern bool drm_detect_hdmi_monitor(struct edid *edid); -extern bool drm_detect_monitor_audio(struct edid *edid); extern int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, @@ -1054,8 +1048,6 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev, extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay);
-extern int drm_edid_header_is_valid(const u8 *raw_edid); -extern unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh, bool rb); diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 6bcaee5..2c0d39a 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -268,4 +268,13 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int len void drm_cache_edid(struct drm_connector *connector, struct edid *edid); unsigned drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags);
+extern bool drm_probe_ddc(struct i2c_adapter *adapter); +extern struct edid *drm_get_edid(struct drm_connector *connector, + struct i2c_adapter *adapter); +extern u8 *drm_find_cea_extension(struct edid *edid); +extern bool drm_detect_hdmi_monitor(struct edid *edid); +extern bool drm_detect_monitor_audio(struct edid *edid); +extern int drm_edid_header_is_valid(const u8 *raw_edid); +extern unsigned drm_edid_is_valid(struct edid *edid, unsigned last_error_flags); + #endif /* __DRM_EDID_H__ */
dri-devel@lists.freedesktop.org
-
Egbert Eich -
Egbert Eich -
Inki Dae -
Jani Nikula -
Sean Paul -
Takashi Iwai -
Ville Syrjälä