DRM/KMS/EDID: Various EDID handling related fixes.
Subsystems (8)
cover DRM/KMS/EDID: Various EDID handling related fixes.
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(-)
Signature · 1 line
1.7.7
PATCH 1/17 DRM/KMS/EDID: Mask out Segment Bits when calculating Offset.
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.
--- drivers/gpu/drm/drm_edid.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/gpu/drm/drm_edid.c +1 -1
@@ -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 ;
Signature · 1 line
1.7.7
PATCH 3/17 DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails.
In reply to Egbert EichIf 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.
--- drivers/gpu/drm/drm_edid.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_edid.c +5 -2
@@ -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;
Signature · 1 line
1.7.7
PATCH 4/17 DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block.
In reply to Egbert EichThere 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.
--- drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_edid.c +13 -0
@@ -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 ; } }
Signature · 1 line
1.7.7
PATCH 5/17 DRM/KMS/EDID: Fix up EEDID Map Blocks if Extension block count has changed.
In reply to Egbert EichEEDID 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.
--- drivers/gpu/drm/drm_edid.c | 105 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 100 insertions(+), 5 deletions(-)
drivers/gpu/drm/drm_edid.c +100 -5
@@ -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; }
Signature · 1 line
1.7.7
Re: [PATCH 05/17] DRM/KMS/EDID: Fix up EEDID Map Blocks if Extension block count has changed.
At Mon, 19 Nov 2012 15:23:06 -0500,
Quoted 19 lines · Egbert Eich
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 <[email protected]>--- drivers/gpu/drm/drm_edid.c | 105 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 100 insertions(+), 5 deletions(-)drivers/gpu/drm/drm_edid.c +0
-0
drivers/gpu/drm/drm_edid.c line 49
Quoted 7 lines
drivers/gpu/drm/drm_edid.c +3
-1@@ -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
PATCH 6/17 DRM/exynos: Fix Memory Leak: free EDID blcok returned by drm_get_edid().
In reply to Egbert Eich--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/gpu/drm/exynos/exynos_hdmi.c +1 -0
@@ -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 ; }
Signature · 1 line
1.7.7
Inki Dae
maintainerRe: [PATCH 06/17] DRM/exynos: Fix Memory Leak: free EDID blcok returned by drm_get_edid().
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 <[email protected]>
Quoted 19 lines
Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/gpu/drm/exynos/exynos_hdmi.c +1
-0@@ -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 ;}
Quoted 8 lines
--
1.7.7_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/1 drm/exynos: fix memory leak: free EDID block
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.
--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/gpu/drm/exynos/exynos_hdmi.c +1 -0
@@ -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 ; }
Signature · 1 line
1.7.7
Re: [PATCH] drm/exynos: fix memory leak: free EDID block
Quoted 16 lines · Egbert Eich
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 variableraw_edid. Therefore it should be freed before the variable goes out of scope.Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/gpu/drm/exynos/exynos_hdmi.c +0
-0
drivers/gpu/drm/exynos/exynos_hdmi.c static int hdmi_get_edid(void *ctx, struct drm_connector *connector, · line 1296
Quoted 4 lines
drivers/gpu/drm/exynos/exynos_hdmi.c +1
-0@@ -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
Quoted 10 lines
drivers/gpu/drm/exynos/exynos_hdmi.c +0
-1}else {return -ENODEV ; } --1.7.7
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: fix memory leak: free EDID block
Quoted 24 lines · Sean Paul
Quoted 16 lines · Egbert Eich
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 variableraw_edid. Therefore it should be freed before the variable goes out of scope.Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/gpu/drm/exynos/exynos_hdmi.c +0
-0drivers/gpu/drm/exynos/exynos_hdmi.c static int hdmi_get_edid(void *ctx, struct drm_connector *connector, · line 1296
Quoted 4 lines
drivers/gpu/drm/exynos/exynos_hdmi.c +1
-0@@ -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.
Quoted 3 lines
The reason this happens is
drm_get_edidattaches this toconnector->display_info.raw_edid, which is then freed in theexynos_drm_connectorfunction that gets the edid.
No, the raw_edid member is gone from struct drm_display_info. All other drivers free the edid data returned by drm_get_edid() themselves.
I would actually prefer if the edid data would be freed by drm_connector_destroy rather than by the drivers as this would allow us to cache an edid without creating copies to pass to the drivers. However some drivers store the pointer to returned edid block somewhere in their own data structures, so if we do this it can easily get out of sync when we reread the edid and it has changed for whatever reason in which case the driver will have a stale and invalid pointer.
Quoted 8 lines
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.
I've looked at the code in the exynos driver in the drm_next branch - there's nothing that destroys this edid data any more.
Cheers,
Egbert.
Sean Paul
Reviewed-byRe: [PATCH] drm/exynos: fix memory leak: free EDID block
Quoted 9 lines · Egbert Eich
Quoted 8 lines · Sean Paul
Quoted 7 lines · Egbert Eich
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 variableraw_edid. Therefore it should be freed before the variable goes out of scope.Signed-off-by Egbert Eich <[email protected]>
Quoted 16 lines
Quoted 16 lines
drivers/gpu/drm/exynos/exynos_hdmi.c static int hdmi_get_edid(void *ctx, struct drm_connector *connector, · line 1296
Quoted 13 lines
--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/gpu/drm/exynos/exynos_hdmi.c +1
-0@@ -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.
Quoted 7 lines
Quoted 3 lines
The reason this happens is
drm_get_edidattaches this toconnector->display_info.raw_edid, which is then freed in theexynos_drm_connectorfunction that gets the edid.No, the
raw_edidmember is gone from structdrm_display_info. All other drivers free the edid data returned bydrm_get_edid()themselves.
A ha! I should have checked first, my apologies.
Quoted 9 lines
I would actually prefer if the edid data would be freed by
drm_connector_destroyrather than by the drivers as this would allow us to cache an edid without creating copies to pass to the drivers. However some drivers store the pointer to returned edid block somewhere in their own data structures, so if we do this it can easily get out of sync when we reread the edid and it has changed for whatever reason in which case the driver will have a stale and invalid pointer.
Agreed, that would be nice.
Quoted 6 lines
Quoted 6 lines
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/
Quoted 6 lines
Quoted 2 lines
For now, please drop this patch.
I've looked at the code in the exynos driver in the
drm_nextbranch - there's nothing that destroys this edid data any more.
Yep, right you are. Your patch does the right thing, thanks for setting me straight :)
I hope we won't need to live with this double-allocation + copy for too long.
Sean
Quoted 2 lines
Cheers,
Egbert.
Re: [PATCH] drm/exynos: fix memory leak: free EDID block
In reply to Sean PaulQuoted 24 lines · Sean Paul
Quoted 16 lines · Egbert Eich
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 variableraw_edid. Therefore it should be freed before the variable goes out of scope.Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/gpu/drm/exynos/exynos_hdmi.c +0
-0drivers/gpu/drm/exynos/exynos_hdmi.c static int hdmi_get_edid(void *ctx, struct drm_connector *connector, · line 1296
Quoted 4 lines
drivers/gpu/drm/exynos/exynos_hdmi.c +1
-0@@ -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.
Quoted 8 lines
The reason this happens is
drm_get_edidattaches this toconnector->display_info.raw_edid, which is then freed in theexynos_drm_connectorfunction 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 <[email protected]> Date: Wed Aug 15 09:32:39 2012 +0000
drm: remove the raw_edid field from struct drm_display_info
BR,
Jani.
Quoted 19 lines
For now, please drop this patch.
Sean
Quoted 10 lines
drivers/gpu/drm/exynos/exynos_hdmi.c +0
-1}else {return -ENODEV ; } --1.7.7
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Inki Dae
maintainerRe: [PATCH] drm/exynos: fix memory leak: free EDID block
In reply to Egbert EichOk, 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 <[email protected]>
Quoted 25 lines
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 variableraw_edid. Therefore it should be freed before the variable goes out of scope.Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/exynos/exynos_hdmi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/gpu/drm/exynos/exynos_hdmi.c +1
-0@@ -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 ;}
Quoted 8 lines
--
1.7.7_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 7/17 DRM/KMS/EDID: Move drm_edid_load.o to drm.ko
In reply to Egbert EichEDIDs 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.
--- drivers/gpu/drm/Makefile | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/gpu/drm/Makefile +1 -1
@@ -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
Signature · 1 line
1.7.7
Re: [PATCH 07/17] DRM/KMS/EDID: Move drm_edid_load.o to drm.ko
At Mon, 19 Nov 2012 15:23:08 -0500,
Quoted 8 lines · Egbert Eich
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 movedrm_edid_loadfrom the helper level to the drm core.Signed-off-by Egbert Eich <[email protected]>
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
Quoted 24 lines
--- drivers/gpu/drm/Makefile | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/gpu/drm/Makefile +1
-1@@ -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.oSignature · 1 line
1.7.7
PATCH 8/17 DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID.
In reply to Egbert Eich--- 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(-)
drivers/gpu/drm/drm_edid.c +64 -13
@@ -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
drivers/gpu/drm/drm_edid_load.c +12 -42
79 lines not on this page · raw message
include/drm/drm_edid.h +1 -0
@@ -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__ */
Signature · 1 line
1.7.7
Re: [PATCH 08/17] DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID.
At Mon, 19 Nov 2012 15:23:09 -0500,
Quoted 27 lines · Egbert Eich
Signed-off-by Egbert Eich <[email protected]>--- 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(-)drivers/gpu/drm/drm_edid.c +11
-0@@ -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 ) {
drivers/gpu/drm/drm_edid.c fixup_blockmaps(u8 **blockp, int eblock_cnt) · line 431
Quoted 10 lines
drivers/gpu/drm/drm_edid.c +10
-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
Quoted 173 lines
drivers/gpu/drm/drm_edid.c +43
-13+} +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 drivers/gpu/drm/drm_edid_load.c +12
-42@@ -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;include/drm/drm_edid.h +1
-0@@ -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__ */ Signature · 1 line
1.7.7
PATCH 9/17 DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID is read.
In reply to Egbert EichFirmware 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.
--- 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(-)
drivers/gpu/drm/drm_crtc_helper.c +1 -5
@@ -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 );
drivers/gpu/drm/drm_edid.c +7 -0
@@ -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 ;
drivers/gpu/drm/drm_edid_load.c +9 -13
@@ -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; }
include/drm/drm_edid.h +3 -1
@@ -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__ */
Signature · 1 line
1.7.7
PATCH 10/17 DRM/KMS/EDID: Cache EDID blobs with extensions.
In reply to Egbert EichAccording 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.
--- 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(-)
drivers/gpu/drm/drm_crtc.c +1 -0
@@ -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--;
drivers/gpu/drm/drm_edid.c +42 -1
70 lines not on this page · raw message
include/drm/drm_crtc.h +1 -0
@@ -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 */
include/drm/drm_edid.h +1 -0
@@ -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__ */
Signature · 1 line
1.7.7
Re: [PATCH 10/17] DRM/KMS/EDID: Cache EDID blobs with extensions.
At Mon, 19 Nov 2012 15:23:11 -0500,
Quoted 43 lines · Egbert Eich
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 <[email protected]>--- 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(-)drivers/gpu/drm/drm_crtc.c +1
-0@@ -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--;drivers/gpu/drm/drm_edid.c +5
-0@@ -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] &&
drivers/gpu/drm/drm_edid.c fixup_edid(u8 **blockp, int valid_extensions) · line 448
Quoted 10 lines
drivers/gpu/drm/drm_edid.c +10
-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().
Quoted 6 lines
drivers/gpu/drm/drm_edid.c +6
-0+ *edidp = new; +return true; + } +return false; +} +
drivers/gpu/drm/drm_edid.c fixup_edid(u8 **blockp, int valid_extensions) · line 464
Quoted 10 lines
drivers/gpu/drm/drm_edid.c +10
-0+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
Quoted 65 lines
drivers/gpu/drm/drm_edid.c +11
-1+ } + 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:include/drm/drm_crtc.h +1
-0@@ -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 */ include/drm/drm_edid.h +1
-0@@ -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__ */ Signature · 1 line
1.7.7
PATCH 11/17 DRM/KMS/EDID: Avoid failing on krealloc on EDID blocks.
In reply to Egbert EichThere 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.
--- drivers/gpu/drm/drm_edid.c | 34 +++++++++++++--------------------- 1 files changed, 13 insertions(+), 21 deletions(-)
drivers/gpu/drm/drm_edid.c +13 -21
@@ -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; }
Signature · 1 line
1.7.7
PATCH 12/17 DRM/KMS/EDID: Consolidate EDID Error Handling.
In reply to Egbert EichConsolidate 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.
--- 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(-)
drivers/gpu/drm/drm_edid.c +61 -40
@@ -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; }
drivers/gpu/drm/radeon/radeon_connectors.c +1 -1
@@ -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 ;
include/drm/drm_crtc.h +1 -3
@@ -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,
include/drm/drm_edid.h +9 -0
@@ -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__ */
Signature · 1 line
1.7.7
PATCH 13/17 DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files.
In reply to Egbert EichSo 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>]..."
--- drivers/gpu/drm/drm_edid_load.c | 45 ++++++++++++++++++++++++++------------ 1 files changed, 31 insertions(+), 14 deletions(-)
drivers/gpu/drm/drm_edid_load.c +31 -14
@@ -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 ;
Signature · 1 line
1.7.7
Re: [PATCH 13/17] DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files.
At Mon, 19 Nov 2012 15:23:14 -0500,
Quoted 33 lines · Egbert Eich
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 <[email protected]>--- drivers/gpu/drm/drm_edid_load.c | 45 ++++++++++++++++++++++++++------------ 1 files changed, 31 insertions(+), 14 deletions(-)drivers/gpu/drm/drm_edid_load.c +5
-3@@ -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 ;
drivers/gpu/drm/drm_edid_load.c struct edid * · line 208
Quoted 10 lines
drivers/gpu/drm/drm_edid_load.c +2
-7- 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
Quoted 37 lines
drivers/gpu/drm/drm_edid_load.c +24
-4+ 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 ;Signature · 1 line
1.7.7
PATCH 14/17 DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid().
In reply to Egbert Eich--- drivers/gpu/drm/ast/ast_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/gpu/drm/ast/ast_mode.c +1 -0
@@ -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"
Signature · 1 line
1.7.7
PATCH 15/17 DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid().
In reply to Egbert Eich--- 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(-)
drivers/gpu/drm/gma500/cdv_intel_dp.c +1 -0
@@ -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); \
drivers/gpu/drm/gma500/oaktrail_lvds.c +1 -0
@@ -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"
drivers/gpu/drm/gma500/psb_intel_modes.c +1 -0
@@ -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" /**
Signature · 1 line
1.7.7
PATCH 16/17 DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid().
In reply to Egbert Eich--- drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/gpu/drm/mgag200/mgag200_mode.c +1 -0
@@ -15,6 +15,7 @@#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h> #include "mgag200_drv.h"
Signature · 1 line
1.7.7
PATCH 17/17 DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.
In reply to Egbert Eich--- include/drm/drm_crtc.h | 8 -------- include/drm/drm_edid.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
include/drm/drm_crtc.h +0 -8
@@ -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);
include/drm/drm_edid.h +9 -0
@@ -267,4 +267,13 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int lenvoid 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__ */
Signature · 1 line
1.7.7
v2 cover DRM/KMS/EDID: Various EDID handling related fixes.
In reply to Egbert EichThe 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(-)
Signature · 1 line
1.7.7
PATCH v2 1/18 DRM/KMS/EDID: Mask out Segment Bits when calculating Offset.
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.
--- drivers/gpu/drm/drm_edid.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/gpu/drm/drm_edid.c +1 -1
@@ -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 ;
Signature · 1 line
1.7.7
PATCH v2 2/18 DRM/KMS/EDID: 0x7e -> EDID_EXTENSION_FLAG_OFFSET (v2)
In reply to Egbert Eichv2: Use offsetof().
--- drivers/gpu/drm/drm_edid.c | 16 +++++++++------- 1 files changed, 9 insertions(+), 7 deletions(-)
drivers/gpu/drm/drm_edid.c +9 -7
@@ -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 :
Signature · 1 line
1.7.7
PATCH v2 3/18 DRM/KMS/EDID: Return Base EDID block if reading EEDID Blocks fails (v2)
In reply to Egbert EichIf 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.
--- drivers/gpu/drm/drm_edid.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_edid.c +5 -2
@@ -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;
Signature · 1 line
1.7.7
PATCH v2 4/18 DRM/KMS/EDID: Don't fail when failing to allocate memory for EDID extensions.
In reply to Egbert EichWhen 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.
--- drivers/gpu/drm/drm_edid.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/drm_edid.c +5 -2
@@ -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++) {
Signature · 1 line
1.7.7
PATCH v2 5/18 DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)
In reply to Egbert EichThere 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.
--- drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_edid.c +13 -0
@@ -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 ; } }
Signature · 1 line
1.7.7
Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)
Quoted 27 lines · Egbert Eich
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 <[email protected]>--- drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)drivers/gpu/drm/drm_edid.c +2
-0@@ -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.
drivers/gpu/drm/drm_edid.c drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) · line 378
Quoted 10 lines
drivers/gpu/drm/drm_edid.c +10
-0+*/ +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.
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)
Quoted 33 lines · Ville Syrj�l�
Quoted 32 lines · Egbert Eich
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 <[email protected]>--- drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)drivers/gpu/drm/drm_edid.c +7
-0@@ -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])
Quoted 8 lines
drivers/gpu/drm/drm_edid.c drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) · line 378
Quoted 5 lines
drivers/gpu/drm/drm_edid.c +5
-0+break ; +if (k ==sizeof (edid_header)) { + valid_extensions =0 ; + goto done_fix_extension_count; + }
memcmp()? Also couldn't we justmemcmp()the whole block against the base block, instead of just the header part?
I don't see an advantage of comparing the entire block with the base block:
the signature should already be unique. However I don't insist ;) Regarding memcmp() you are definitely right, I will change the code.
Quoted 3 lines
Also the comment is somehow misleading. It talks about the base block even though we're looking at the extension block.
Reason for this patch:
I had a bug report for a monitor announcing extension blocks in the extension block flag of the base block (over 200!) although it wasn't EDDC capable. For some reason it got past the ACK check when the segment number was written to address 0x30 and happily transferred the base block for any odd numbered block and some garbage for even ones.
The only reliable way we found to catch this condition early was to check if block 2 had the header of a base block which will happen when the display cannot deal with the segment number.
This was what I tried to summarize in very few words - maybe i should reword it a bit.
Thanks!
Egbert.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)
Quoted 43 lines · Egbert Eich
Quoted 33 lines · Ville Syrj�l�
Quoted 32 lines · Egbert Eich
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 <[email protected]>--- drivers/gpu/drm/drm_edid.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)drivers/gpu/drm/drm_edid.c +7
-0@@ -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])Quoted 8 lines
drivers/gpu/drm/drm_edid.c drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) · line 378
Quoted 5 lines
drivers/gpu/drm/drm_edid.c +5
-0+break ; +if (k ==sizeof (edid_header)) { + valid_extensions =0 ; + goto done_fix_extension_count; + }
memcmp()? Also couldn't we justmemcmp()the whole block against the base block, instead of just the header part?
Quoted 2 lines
I don't see an advantage of comparing the entire block with the base block:
the signature should already be unique. However I don't insist ;)
Me neither. I just figured it might reduce the chance of false positives. But if you say that can't happen, I'll take your word for it.
Quoted 7 lines
Regarding
memcmp()you are definitely right, I will change the code.Quoted 3 lines
Also the comment is somehow misleading. It talks about the base block even though we're looking at the extension block.
Quoted 6 lines
Reason for this patch:
I had a bug report for a monitor announcing extension blocks in the extension block flag of the base block (over 200!) although it wasn't EDDC capable. For some reason it got past the ACK check when the segment number was written to address0x30and happily transferred the base block for any odd numbered block and some garbage for even ones.
That's nasty. When I saw your patch I was immediately thinking that this is caused by the IGNORE_NAK flag, but then I read the current code, and realized that we're not using that flag. It was used in the original EDDC patch, and I was worried that this kind of bad behaviour would be possible if use the flag. But it seems I underestimated how crappy the monitor hardwar can be.
Quoted 6 lines
The only reliable way we found to catch this condition early was to check if block 2 had the header of a base block which will happen when the display cannot deal with the segment number.
This was what I tried to summarize in very few words - maybe i should reword it a bit.
Right. The idea seems reasonable, I just found the comment somehow a bit confusing when I was reading the code following it.
So maybe something like 'Test if base block ..., by checking whether the extension block is a duplicate the base block.' Although the use of memcmp() will already make things much clearer.
Signature · 2 lines
Ville Syrjälä Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 05/18] DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v2)
Quoted 19 lines · Ville Syrj�l�
Me neither. I just figured it might reduce the chance of false positives. But if you say that can't happen, I'll take your word for it.
Quoted 7 lines
Regarding
memcmp()you are definitely right, I will change the code.Quoted 3 lines
Also the comment is somehow misleading. It talks about the base block even though we're looking at the extension block.
Quoted 6 lines
Reason for this patch:
I had a bug report for a monitor announcing extension blocks in the extension block flag of the base block (over 200!) although it wasn't EDDC capable. For some reason it got past the ACK check when the segment number was written to address0x30and happily transferred the base block for any odd numbered block and some garbage for even ones.
Quoted 6 lines
That's nasty. When I saw your patch I was immediately thinking that this is caused by the
IGNORE_NAKflag, but then I read the current code, and realized that we're not using that flag. It was used in the original EDDC patch, and I was worried that this kind of bad behaviour would be possible if use the flag. But it seems I underestimated how crappy the monitor hardwar can be.
Right.
This flag would only make sense for the base block (to reset an EDDC capable monitor but not fail on non-EDDC capable ones). In fact I had a patch to add this, but then I realized that none of the I2C implemenations in the gfx drivers honored this flag. But then I reread the VESA spec for EDDC, this requires that the segment address in the monitor has to be reset on a STOP condition.
So if there ever is the case that a monitor has a segment > 0 selected when drm_read_edid() is called for the first time (because of a system crash for instance) the validation will fail and causes a 2nd read attempt for which the segment address should be reset by the previous transfer.
Regarding crappiness of monitor hardware: catch me at a bar and I can tell you more stories ... ;p
Quoted 8 lines
Quoted 7 lines
The only reliable way we found to catch this condition early was to check if block 2 had the header of a base block which will happen when the display cannot deal with the segment number.
This was what I tried to summarize in very few words - maybe i should reword it a bit.
Quoted 6 lines
Right. The idea seems reasonable, I just found the comment somehow a bit confusing when I was reading the code following it.
So maybe something like 'Test if base block ..., by checking whether the extension block is a duplicate the base block.' Although the use of
memcmp()will already make things much clearer.
Right. I've already reworked it. Will send out.
Cheers,
Egbert.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 1/1 DRM/KMS/EDID: Test EDDC if EDID announces more than one Extension Block (v3)
In reply to Egbert EichThere 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ä <[email protected]>:
Reworded comment,
Used memcmp(),
Compared entire base block instead of signature only.
--- drivers/gpu/drm/drm_edid.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_edid.c +10 -0
@@ -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 ; } }
Signature · 6 lines
1.7.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v2 6/18 DRM/KMS/EDID: Don't expect extension blocks for EDID Versions < 1.3.
In reply to Egbert EichEDID 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.
--- drivers/gpu/drm/drm_edid.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_edid.c +6 -0
@@ -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",
Signature · 1 line
1.7.7
PATCH v2 7/18 DRM/KMS/EDID: Don't reallocate EDID blob when size has shrunk.
In reply to Egbert Eichvalid_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.
--- drivers/gpu/drm/drm_edid.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
drivers/gpu/drm/drm_edid.c +0 -4
@@ -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;
Signature · 1 line
1.7.7
PATCH v2 8/18 DRM/KMS/EDID: Fix up EEDID Map Blogs if Extension Block Count has changed (v2)
In reply to Egbert EichEEDID 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.
--- drivers/gpu/drm/drm_edid.c | 89 ++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 89 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_edid.c +89 -0
@@ -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; }
Signature · 1 line
1.7.7
PATCH v2 9/18 DRM/KMS/EDID: Move drm_edid_load.o to drm.ko (v2)
In reply to Egbert EichEDIDs 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'
--- drivers/gpu/drm/Kconfig | 3 ++- drivers/gpu/drm/Makefile | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/Kconfig +2 -1
@@ -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
drivers/gpu/drm/Makefile +1 -1
@@ -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
Signature · 1 line
1.7.7
PATCH v2 10/18 DRM/KMS/EDID: Feed 'firmware' supplied EDID blocks whenever the EDID is read (v2)
In reply to Egbert EichFirmware 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.
--- 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(-)
drivers/gpu/drm/drm_crtc_helper.c +1 -5
@@ -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 );
drivers/gpu/drm/drm_edid.c +7 -0
@@ -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 ;
drivers/gpu/drm/drm_edid_load.c +9 -13
@@ -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; }
include/drm/drm_edid.h +3 -1
@@ -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__ */
Signature · 1 line
1.7.7
PATCH v2 11/18 DRM/KMS/EDID: Allow for multiple Connectors when specifying 'firmware'-EDID Files (v2)
In reply to Egbert EichSo 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.
--- drivers/gpu/drm/drm_edid_load.c | 45 ++++++++++++++++++++++++++------------ 1 files changed, 31 insertions(+), 14 deletions(-)
drivers/gpu/drm/drm_edid_load.c +31 -14
@@ -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 ;
Signature · 1 line
1.7.7
PATCH v2 12/18 DRM/KMS/EDID: Use Extension Block Fixup Code also for 'firmware' EDID (v2)
In reply to Egbert Eichin 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().
--- 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(-)
drivers/gpu/drm/drm_edid.c +58 -6
@@ -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
drivers/gpu/drm/drm_edid_load.c +12 -42
79 lines not on this page · raw message
include/drm/drm_edid.h +1 -0
@@ -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__ */
Signature · 1 line
1.7.7
PATCH v2 13/18 DRM/KMS/EDID: Cache EDID blobs with extensions (v2)
In reply to Egbert EichAccording 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.
--- 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(-)
drivers/gpu/drm/drm_crtc.c +1 -0
@@ -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--;
drivers/gpu/drm/drm_edid.c +49 -8
107 lines not on this page · raw message
include/drm/drm_crtc.h +1 -0
@@ -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 */
include/drm/drm_edid.h +1 -0
@@ -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__ */
Signature · 1 line
1.7.7
Re: [PATCH v2 13/18] DRM/KMS/EDID: Cache EDID blobs with extensions (v2)
Quoted 101 lines · Egbert Eich
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 <[email protected]>--- 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(-)drivers/gpu/drm/drm_crtc.c +1
-0@@ -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--;drivers/gpu/drm/drm_edid.c +38
-4@@ -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;
drivers/gpu/drm/drm_edid.c drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) · line 489
Quoted 10 lines
drivers/gpu/drm/drm_edid.c +2
-2} }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;
Shouldn't you erase the cache here too?
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH v2 13/18] DRM/KMS/EDID: Cache EDID blobs with extensions (v2)
Quoted 9 lines · Ville Syrj�l�
drivers/gpu/drm/drm_edid.c drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) · line 489
Quoted 5 lines · Egbert Eich
drivers/gpu/drm/drm_edid.c +1
-1-/* 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?
Indeed. And I had fixed it. It went into the wrong patch however :(
Will correct.
Thanks,
Egbert.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 1/1 DRM/KMS/EDID: Cache EDID blobs with extensions (v3)
In reply to Egbert EichAccording 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ä <[email protected]>.
--- 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(-)
drivers/gpu/drm/drm_crtc.c +1 -0
@@ -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--;
drivers/gpu/drm/drm_edid.c +51 -9
110 lines not on this page · raw message
include/drm/drm_crtc.h +1 -0
@@ -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 */
include/drm/drm_edid.h +1 -0
@@ -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__ */
Signature · 6 lines
1.7.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v2 14/18 DRM/KMS/EDID: Consolidate EDID Error Handling (v2)
In reply to Egbert EichConsolidate 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.
--- 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(-)
drivers/gpu/drm/drm_edid.c +69 -46
@@ -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; }
drivers/gpu/drm/radeon/radeon_connectors.c +1 -1
@@ -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 ;
include/drm/drm_crtc.h +1 -3
@@ -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,
include/drm/drm_edid.h +10 -0
@@ -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__ */
Signature · 1 line
1.7.7
PATCH v3 1/1 DRM/KMS/EDID: Consolidate EDID Error Handling (v3)
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.
--- 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(-)
drivers/gpu/drm/drm_edid.c +70 -47
@@ -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; }
drivers/gpu/drm/radeon/radeon_connectors.c +1 -1
@@ -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 ;
include/drm/drm_crtc.h +1 -3
@@ -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,
include/drm/drm_edid.h +10 -0
@@ -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__ */
Signature · 1 line
1.7.7
Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)
Quoted 28 lines · Egbert Eich
Consolidate the
null_edid_counterand thebad_edid_counterinto 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 <[email protected]>--- 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(-)drivers/gpu/drm/drm_edid.c +0
-0@@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid) }EXPORT_SYMBOL (drm_edid_header_is_valid);
drivers/gpu/drm/drm_edid.c int drm_edid_header_is_valid(const u8 *raw_edid) · line 168
Quoted 10 lines
drivers/gpu/drm/drm_edid.c +9
-0+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.
Quoted 72 lines
drivers/gpu/drm/drm_edid.c +17
-7+} +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); }
drivers/gpu/drm/drm_edid.c bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) · line 252
Quoted 10 lines
drivers/gpu/drm/drm_edid.c +9
-1-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();
drivers/gpu/drm/drm_edid.c bool drm_edid_is_valid(struct edid *edid) · line 271
Quoted 9 lines
drivers/gpu/drm/drm_edid.c +1
-1}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().
Quoted 80 lines
drivers/gpu/drm/drm_edid.c +12
-20return 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) {
drivers/gpu/drm/drm_edid.c drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) · line 534
Quoted 5 lines
drivers/gpu/drm/drm_edid.c +1
-1@@ -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.
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)
Quoted 18 lines · Ville Syrj�l�
Quoted 18 lines
drivers/gpu/drm/drm_edid.c +9
-0@@ -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;
[...]
Quoted 5 lines
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.
Quoted 9 lines
drivers/gpu/drm/drm_edid.c bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) · line 252
Quoted 7 lines
drivers/gpu/drm/drm_edid.c +7
-0+ +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();
Right.
It's stupid anyway. See below.
Quoted 7 lines
Quoted 7 lines
}
EXPORT_SYMBOL(drm_edid_block_valid);drivers/gpu/drm/drm_edid.c +0
-0@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)return false;
Quoted 6 lines
drivers/gpu/drm/drm_edid.c bool drm_edid_is_valid(struct edid *edid) · line 271
Quoted 3 lines
drivers/gpu/drm/drm_edid.c +1
-1for (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'
Oops, right. Leftover from old code.
Quoted 2 lines
change seems superfluous since you're not using the more detailed return value from
drm_edid_block_check_error().
This should probably be changed.
For the drm_edid_block_valid() I'm already using the previous error state as argument - but I don't tell the result of this read. Doesn't make much sense :(
Something similar should be done for drm_edid_is_valid() - even if the driver doesn't bother (for instance because this function is only called once when the device structures are initialized).
The current code ignores the error state for extension blocks i guess it should not if we want to avoid having repreated logging of errors in the extension blocks.
What should be done is:
unsigned err = drm_edid_block_check_error();
result |= err;
if (!err) {
valid_extensions++;
...
}Quoted 5 lines
drivers/gpu/drm/drm_edid.c drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) · line 534
Quoted 2 lines
drivers/gpu/drm/drm_edid.c +1
-1-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.
Exactly. Please see above.
Thanks for looking at the patches!
Cheers,
Egbert.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)
Quoted 63 lines · Egbert Eich
Quoted 18 lines · Ville Syrj�l�
Quoted 18 lines
drivers/gpu/drm/drm_edid.c +9
-0@@ -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;[...]
Quoted 5 lines
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.Quoted 9 lines
drivers/gpu/drm/drm_edid.c bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) · line 252
Quoted 7 lines
drivers/gpu/drm/drm_edid.c +7
-0+ +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();
Right.
It's stupid anyway. See below.Quoted 7 lines
Quoted 7 lines
}
EXPORT_SYMBOL(drm_edid_block_valid);drivers/gpu/drm/drm_edid.c +0
-0@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)return false;Quoted 6 lines
drivers/gpu/drm/drm_edid.c bool drm_edid_is_valid(struct edid *edid) · line 271
Quoted 3 lines
drivers/gpu/drm/drm_edid.c +1
-1for (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'
Oops, right. Leftover from old code.
Quoted 2 lines
change seems superfluous since you're not using the more detailed return value from
drm_edid_block_check_error().This should probably be changed.
For thedrm_edid_block_valid()I'm already using the previous error state as argument - but I don't tell the result of this read. Doesn't make much sense :(
Quoted 7 lines
Something similar should be done for
drm_edid_is_valid()- even if the driver doesn't bother (for instance because this function is only called once when the device structures are initialized).The current code ignores the error state for extension blocks i guess it should not if we want to avoid having repreated logging of errors in the extension blocks.
I'm not sure. The current code dump all failed extension block, doesn't it? Maybe we actually do want that to happen, at least w/ debugs enabled.
Then there are various retry loops in the code, which may or may not be necessary. I have a feeling some of them may have been attempts at papering over a bug that I fixed [1] in the i2c bitbanging code. But if they are necessary, I'm not sure we really appreciate repeated dumps of the same block.
Signature · 2 lines
Ville Syrjälä Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)
Quoted 6 lines · Ville Syrj�l�
Quoted 5 lines · Egbert Eich
Something similar should be done for
drm_edid_is_valid()- even if the driver doesn't bother (for instance because this function is only called once when the device structures are initialized).
Quoted 7 lines
Quoted 3 lines
The current code ignores the error state for extension blocks i guess it should not if we want to avoid having repreated logging of errors in the extension blocks.
I'm not sure. The current code dump all failed extension block, doesn't it? Maybe we actually do want that to happen, at least w/ debugs enabled.
Yes, it does. It drops them silently. I was a bit unclear - what I meant was: we ignore the result for the final error state we keep to be able to track which errors we still want to log.
Quoted 8 lines
Then there are various retry loops in the code, which may or may not be necessary. I have a feeling some of them may have been attempts at papering over a bug that I fixed [1] in the i2c bitbanging code. But if they are necessary, I'm not sure we really appreciate repeated dumps of the same block.
Right, I know this patch :)
I'm also not sure about the retries. I guess for the base block we want at least one retry in case the display is in an intermediate state where it will not send the base block.
Apart from this I've seen many EDID failures however I don't recall seeing one which got fixed by repeated reads. However to see if those loops fix any issue we may want to keep all messages enabled for the time being.
Cheers,
Egbert.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v4 1/1 DRM/KMS/EDID: Consolidate EDID Error Handling (v4)
In reply to Egbert EichConsolidate 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.
--- 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(-)
drivers/gpu/drm/drm_edid.c +80 -56
@@ -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 ;
drivers/gpu/drm/radeon/radeon_atombios.c +1 -1
@@ -1644,7 +1644,7 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(structmemcpy ((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
drivers/gpu/drm/radeon/radeon_connectors.c +1 -1
@@ -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 ;
include/drm/drm_crtc.h +2 -4
@@ -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);
include/drm/drm_edid.h +10 -0
@@ -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__ */
Signature · 1 line
1.7.7
PATCH v5 1/1 DRM/KMS/EDID: Consolidate EDID Error Handling (v5)
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.
--- 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(-)
drivers/gpu/drm/drm_edid.c +80 -56
@@ -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 ;
drivers/gpu/drm/radeon/radeon_atombios.c +1 -1
@@ -1644,7 +1644,7 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(structmemcpy ((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
drivers/gpu/drm/radeon/radeon_combios.c +1 -1
@@ -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; }
drivers/gpu/drm/radeon/radeon_connectors.c +1 -1
@@ -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 ;
include/drm/drm_crtc.h +2 -4
@@ -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);
include/drm/drm_edid.h +10 -0
@@ -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__ */
Signature · 6 lines
1.7.7 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v2 15/18 DRM/KMS/ast: Include drm_edid.h in file using drm_get_edid().
In reply to Egbert Eich--- drivers/gpu/drm/ast/ast_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/gpu/drm/ast/ast_mode.c +1 -0
@@ -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"
Signature · 1 line
1.7.7
PATCH v2 16/18 DRM/KMS/gma500: Include drm_edid.h in file using drm_get_edid().
In reply to Egbert Eich--- 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(-)
drivers/gpu/drm/gma500/cdv_intel_dp.c +1 -0
@@ -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); \
drivers/gpu/drm/gma500/oaktrail_lvds.c +1 -0
@@ -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"
drivers/gpu/drm/gma500/psb_intel_modes.c +1 -0
@@ -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" /**
Signature · 1 line
1.7.7
PATCH v2 17/18 DRM/KMS/mgag200: Include drm_edid.h in file using drm_get_edid().
In reply to Egbert Eich--- drivers/gpu/drm/mgag200/mgag200_mode.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/gpu/drm/mgag200/mgag200_mode.c +1 -0
@@ -15,6 +15,7 @@#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_edid.h> #include "mgag200_drv.h"
Signature · 1 line
1.7.7
PATCH v2 18/18 DRM/KMS/EDID: Move EDID related Functions to drm_edid.h.
In reply to Egbert Eich--- include/drm/drm_crtc.h | 8 -------- include/drm/drm_edid.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
include/drm/drm_crtc.h +0 -8
@@ -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);
include/drm/drm_edid.h +9 -0
@@ -268,4 +268,13 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int lenvoid 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__ */
Signature · 1 line
1.7.7
PATCH v4 1/1 DRM/KMS/EDID: Move EDID related Functions to drm_edid.h (v2)
In reply to Egbert Eichv2: Adjusted to apply cleanly.
--- include/drm/drm_crtc.h | 8 -------- include/drm/drm_edid.h | 9 +++++++++ 2 files changed, 9 insertions(+), 8 deletions(-)
include/drm/drm_crtc.h +0 -8
@@ -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);
include/drm/drm_edid.h +9 -0
@@ -268,4 +268,13 @@ int drm_validate_edid_blob(struct drm_connector *connector, u8 **blockp, int lenvoid 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__ */
Signature · 1 line
1.7.7