Hello,
This small patch series attemps at decoupling EDID retrieval from drm_connector, following a discussion with Daniel Vetter [1]. While working on this I noticed a few issues with EDID retrieval, which I have attempted to fix in patches 1/5 to 4/5. Patch 5/5 then tries to decouple the EDID retrieval from drm_connector, in what I think is a pretty bad way. I would like to discuss this further to see if a) there's an interest, and b) someone has a better idea :-)
Regardless of the outcome of the discussion on patch 5/5, the first four patches are candidates for upstreaming as they try to address existing issues (they may also introduce new bugs, but hopefully they will be pointed out during review).
The patches are available at
git://linuxtv.org/pinchartl/media.git omapdrm/edid
[1] https://lists.freedesktop.org/archives/dri-devel/2019-August/231930.html
Laurent Pinchart (5): drm/edid: Reorganise the DisplayID parsing code drm/edid: Move functions to avoid forward declaration drm/edid: Move DisplayID tile parsing to drm_connector.c drm/edid: Honour connector->force in drm_do_get_edid() [HACK] drm/edid: Decouple EDID retrieval from connector
drivers/gpu/drm/drm_connector.c | 137 +++- drivers/gpu/drm/drm_dp_mst_topology.c | 3 +- drivers/gpu/drm/drm_edid.c | 857 ++++++++++++-------------- include/drm/drm_connector.h | 3 +- include/drm/drm_displayid.h | 2 + include/drm/drm_edid.h | 2 + 6 files changed, 534 insertions(+), 470 deletions(-)
This change started as an attempt to remove the forward declaration of validate_displayid(), and ended up reorganising the DisplayID parsing code to fix serial intertwined issues.
The drm_parse_display_id() function, which parses a DisplayID block, is made aware of whether the DisplayID comes from an EDID extension block or is direct DisplayID data. This is a layering violation, this should be handled in the caller. Similarly, the validate_displayid() function should not take an offset in the data buffer, it should also receive a pointer to the DisplayID data.
In order to simplify the callers of drm_find_displayid_extension(), the function is modified to return a pointer to the DisplayID data, not to the EDID extension block. The pointer can then be used directly for validation and parsing, without the need to add an offset.
For consistency reasons the validate_displayid() function is renamed to drm_displayid_is_valid() and modified to return a bool, and the drm_parse_display_id() renamed to drm_parse_displayid().
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_edid.c | 104 ++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 52 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 82a4ceed3fcf..7c6bc5183b60 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1342,7 +1342,6 @@ MODULE_PARM_DESC(edid_fixup,
static void drm_get_displayid(struct drm_connector *connector, struct edid *edid); -static int validate_displayid(u8 *displayid, int length, int idx);
static int drm_edid_block_checksum(const u8 *raw_edid) { @@ -2927,17 +2926,49 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id) return edid_ext; }
+static bool drm_displayid_is_valid(u8 *displayid, unsigned int length) +{ + struct displayid_hdr *base; + u8 csum = 0; + int i; + + base = (struct displayid_hdr *)displayid; + + DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n", + base->rev, base->bytes, base->prod_id, base->ext_count); + + if (base->bytes + 5 > length) + return false; + + for (i = 0; i <= base->bytes + 5; i++) + csum += displayid[i]; + + if (csum) { + DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum); + return false; + } + + return true; +}
static u8 *drm_find_displayid_extension(const struct edid *edid) { - return drm_find_edid_extension(edid, DISPLAYID_EXT); + u8 *ext = drm_find_edid_extension(edid, DISPLAYID_EXT); + + if (!ext) + return NULL; + + /* + * Skip the EDID extension block tag number to return the DisplayID + * data. + */ + return ext + 1; }
static u8 *drm_find_cea_extension(const struct edid *edid) { - int ret; - int idx = 1; - int length = EDID_LENGTH; + int idx; + int length = EDID_LENGTH - 1; struct displayid_block *block; u8 *cea; u8 *displayid; @@ -2952,11 +2983,10 @@ static u8 *drm_find_cea_extension(const struct edid *edid) if (!displayid) return NULL;
- ret = validate_displayid(displayid, length, idx); - if (ret) + if (!drm_displayid_is_valid(displayid, length)) return NULL;
- idx += sizeof(struct displayid_hdr); + idx = sizeof(struct displayid_hdr); for_each_displayid_db(displayid, block, idx, length) { if (block->tag == DATA_BLOCK_CTA) { cea = (u8 *)block; @@ -4711,29 +4741,6 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi return quirks; }
-static int validate_displayid(u8 *displayid, int length, int idx) -{ - int i; - u8 csum = 0; - struct displayid_hdr *base; - - base = (struct displayid_hdr *)&displayid[idx]; - - DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n", - base->rev, base->bytes, base->prod_id, base->ext_count); - - if (base->bytes + 5 > length - idx) - return -EINVAL; - for (i = idx; i <= base->bytes + 5; i++) { - csum += displayid[i]; - } - if (csum) { - DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum); - return -EINVAL; - } - return 0; -} - static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev, struct displayid_detailed_timings_1 *timings) { @@ -4809,9 +4816,8 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, struct edid *edid) { u8 *displayid; - int ret; - int idx = 1; - int length = EDID_LENGTH; + int idx; + int length = EDID_LENGTH - 1; struct displayid_block *block; int num_modes = 0;
@@ -4819,11 +4825,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, if (!displayid) return 0;
- ret = validate_displayid(displayid, length, idx); - if (ret) + if (!drm_displayid_is_valid(displayid, length)) return 0;
- idx += sizeof(struct displayid_hdr); + idx = sizeof(struct displayid_hdr); for_each_displayid_db(displayid, block, idx, length) { switch (block->tag) { case DATA_BLOCK_TYPE_1_DETAILED_TIMING: @@ -5424,23 +5429,17 @@ static int drm_parse_tiled_block(struct drm_connector *connector, return 0; }
-static int drm_parse_display_id(struct drm_connector *connector, - u8 *displayid, int length, - bool is_edid_extension) +static int drm_parse_displayid(struct drm_connector *connector, + u8 *displayid, int length) { - /* if this is an EDID extension the first byte will be 0x70 */ - int idx = 0; struct displayid_block *block; + int idx; int ret;
- if (is_edid_extension) - idx = 1; + if (!drm_displayid_is_valid(displayid, length)) + return -EINVAL;
- ret = validate_displayid(displayid, length, idx); - if (ret) - return ret; - - idx += sizeof(struct displayid_hdr); + idx = sizeof(struct displayid_hdr); for_each_displayid_db(displayid, block, idx, length) { DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n", block->tag, block->rev, block->num_bytes); @@ -5468,8 +5467,9 @@ static int drm_parse_display_id(struct drm_connector *connector, static void drm_get_displayid(struct drm_connector *connector, struct edid *edid) { - void *displayid = NULL; + void *displayid; int ret; + connector->has_tile = false; displayid = drm_find_displayid_extension(edid); if (!displayid) { @@ -5477,16 +5477,16 @@ static void drm_get_displayid(struct drm_connector *connector, goto out_drop_ref; }
- ret = drm_parse_display_id(connector, displayid, EDID_LENGTH, true); + ret = drm_parse_displayid(connector, displayid, EDID_LENGTH - 1); if (ret < 0) goto out_drop_ref; if (!connector->has_tile) goto out_drop_ref; return; + out_drop_ref: if (connector->tile_group) { drm_mode_put_tile_group(connector->dev, connector->tile_group); connector->tile_group = NULL; } - return; }
On Wed, Aug 21, 2019 at 09:50:01PM +0300, Laurent Pinchart wrote:
This change started as an attempt to remove the forward declaration of validate_displayid(), and ended up reorganising the DisplayID parsing code to fix serial intertwined issues.
The drm_parse_display_id() function, which parses a DisplayID block, is made aware of whether the DisplayID comes from an EDID extension block or is direct DisplayID data. This is a layering violation, this should be handled in the caller. Similarly, the validate_displayid() function should not take an offset in the data buffer, it should also receive a pointer to the DisplayID data.
In order to simplify the callers of drm_find_displayid_extension(), the function is modified to return a pointer to the DisplayID data, not to the EDID extension block. The pointer can then be used directly for validation and parsing, without the need to add an offset.
For consistency reasons the validate_displayid() function is renamed to drm_displayid_is_valid() and modified to return a bool, and the drm_parse_display_id() renamed to drm_parse_displayid().
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/drm_edid.c | 104 ++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 52 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 82a4ceed3fcf..7c6bc5183b60 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1342,7 +1342,6 @@ MODULE_PARM_DESC(edid_fixup,
static void drm_get_displayid(struct drm_connector *connector, struct edid *edid); -static int validate_displayid(u8 *displayid, int length, int idx);
static int drm_edid_block_checksum(const u8 *raw_edid) { @@ -2927,17 +2926,49 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id) return edid_ext; }
+static bool drm_displayid_is_valid(u8 *displayid, unsigned int length)
const* would be nice.
same in many other places in the series.
+{
- struct displayid_hdr *base;
- u8 csum = 0;
- int i;
- base = (struct displayid_hdr *)displayid;
Can be done when declaring the variable.
- DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
base->rev, base->bytes, base->prod_id, base->ext_count);- if (base->bytes + 5 > length)
return false;- for (i = 0; i <= base->bytes + 5; i++)
csum += displayid[i];- if (csum) {
DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);return false;- }
- return true;
+}
static u8 *drm_find_displayid_extension(const struct edid *edid) {
- return drm_find_edid_extension(edid, DISPLAYID_EXT);
- u8 *ext = drm_find_edid_extension(edid, DISPLAYID_EXT);
- if (!ext)
return NULL;- /*
* Skip the EDID extension block tag number to return the DisplayID* data.*/- return ext + 1;
}
static u8 *drm_find_cea_extension(const struct edid *edid) {
- int ret;
- int idx = 1;
- int length = EDID_LENGTH;
- int idx;
- int length = EDID_LENGTH - 1; struct displayid_block *block; u8 *cea; u8 *displayid;
@@ -2952,11 +2983,10 @@ static u8 *drm_find_cea_extension(const struct edid *edid) if (!displayid) return NULL;
- ret = validate_displayid(displayid, length, idx);
- if (ret)
- if (!drm_displayid_is_valid(displayid, length)) return NULL;
- idx += sizeof(struct displayid_hdr);
- idx = sizeof(struct displayid_hdr);
It's a bit silly/fragile that everyone has to do this. I'd rather eliminate this idx initialzation from the callers of for_each_displayid_db() entirely.
for_each_displayid_db(displayid, block, idx, length) { if (block->tag == DATA_BLOCK_CTA) { cea = (u8 *)block;
This here is also borked. We should validate the size of the CEA ext block somewhere.
@@ -4711,29 +4741,6 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi return quirks; }
-static int validate_displayid(u8 *displayid, int length, int idx) -{
- int i;
- u8 csum = 0;
- struct displayid_hdr *base;
- base = (struct displayid_hdr *)&displayid[idx];
- DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
base->rev, base->bytes, base->prod_id, base->ext_count);- if (base->bytes + 5 > length - idx)
return -EINVAL;- for (i = idx; i <= base->bytes + 5; i++) {
csum += displayid[i];- }
- if (csum) {
DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);return -EINVAL;- }
- return 0;
-}
static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev, struct displayid_detailed_timings_1 *timings) { @@ -4809,9 +4816,8 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, struct edid *edid) { u8 *displayid;
- int ret;
- int idx = 1;
- int length = EDID_LENGTH;
- int idx;
- int length = EDID_LENGTH - 1; struct displayid_block *block; int num_modes = 0;
@@ -4819,11 +4825,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, if (!displayid) return 0;
- ret = validate_displayid(displayid, length, idx);
- if (ret)
- if (!drm_displayid_is_valid(displayid, length)) return 0;
- idx += sizeof(struct displayid_hdr);
- idx = sizeof(struct displayid_hdr); for_each_displayid_db(displayid, block, idx, length) { switch (block->tag) { case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
@@ -5424,23 +5429,17 @@ static int drm_parse_tiled_block(struct drm_connector *connector, return 0; }
-static int drm_parse_display_id(struct drm_connector *connector,
u8 *displayid, int length,bool is_edid_extension)+static int drm_parse_displayid(struct drm_connector *connector,
u8 *displayid, int length){
- /* if this is an EDID extension the first byte will be 0x70 */
- int idx = 0; struct displayid_block *block;
- int idx; int ret;
- if (is_edid_extension)
idx = 1;
- if (!drm_displayid_is_valid(displayid, length))
return -EINVAL;
- ret = validate_displayid(displayid, length, idx);
- if (ret)
return ret;- idx += sizeof(struct displayid_hdr);
- idx = sizeof(struct displayid_hdr); for_each_displayid_db(displayid, block, idx, length) { DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n", block->tag, block->rev, block->num_bytes);
@@ -5468,8 +5467,9 @@ static int drm_parse_display_id(struct drm_connector *connector, static void drm_get_displayid(struct drm_connector *connector, struct edid *edid) {
- void *displayid = NULL;
- void *displayid; int ret;
- connector->has_tile = false; displayid = drm_find_displayid_extension(edid); if (!displayid) {
@@ -5477,16 +5477,16 @@ static void drm_get_displayid(struct drm_connector *connector, goto out_drop_ref; }
- ret = drm_parse_display_id(connector, displayid, EDID_LENGTH, true);
- ret = drm_parse_displayid(connector, displayid, EDID_LENGTH - 1); if (ret < 0) goto out_drop_ref; if (!connector->has_tile) goto out_drop_ref; return;
out_drop_ref: if (connector->tile_group) { drm_mode_put_tile_group(connector->dev, connector->tile_group); connector->tile_group = NULL; }
- return;
}
Regards,
Laurent Pinchart
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Move the EDID retrieval functions to the end of the file to avoid forward declarations. While at it fix a typo in a comment (s/firmare/firmware/). No functional change is included.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_edid.c | 637 ++++++++++++++++++------------------- 1 file changed, 317 insertions(+), 320 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7c6bc5183b60..bfcb232b9760 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1340,9 +1340,6 @@ module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, "Minimum number of valid EDID header bytes (0-8, default 6)");
-static void drm_get_displayid(struct drm_connector *connector, - struct edid *edid); - static int drm_edid_block_checksum(const u8 *raw_edid) { int i; @@ -1481,323 +1478,6 @@ bool drm_edid_is_valid(struct edid *edid) } EXPORT_SYMBOL(drm_edid_is_valid);
-#define DDC_SEGMENT_ADDR 0x30 -/** - * drm_do_probe_ddc_edid() - get EDID information via I2C - * @data: I2C device adapter - * @buf: EDID data buffer to be filled - * @block: 128 byte EDID block to start fetching from - * @len: EDID data buffer length to fetch - * - * Try to fetch EDID information by calling I2C driver functions. - * - * Return: 0 on success or -1 on failure. - */ -static int -drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len) -{ - struct i2c_adapter *adapter = data; - unsigned char start = block * EDID_LENGTH; - unsigned char segment = block >> 1; - unsigned char xfers = segment ? 3 : 2; - int ret, retries = 5; - - /* - * The core I2C driver will automatically retry the transfer if the - * adapter reports EAGAIN. However, we find that bit-banging transfers - * are susceptible to errors under a heavily loaded machine and - * generate spurious NAKs and timeouts. Retrying the transfer - * of the individual block a few times seems to overcome this. - */ - do { - struct i2c_msg msgs[] = { - { - .addr = DDC_SEGMENT_ADDR, - .flags = 0, - .len = 1, - .buf = &segment, - }, { - .addr = DDC_ADDR, - .flags = 0, - .len = 1, - .buf = &start, - }, { - .addr = DDC_ADDR, - .flags = I2C_M_RD, - .len = len, - .buf = buf, - } - }; - - /* - * Avoid sending the segment addr to not upset non-compliant - * DDC monitors. - */ - ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers); - - if (ret == -ENXIO) { - DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", - adapter->name); - break; - } - } while (ret != xfers && --retries); - - return ret == xfers ? 0 : -1; -} - -static void connector_bad_edid(struct drm_connector *connector, - u8 *edid, int num_blocks) -{ - int i; - - if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) - return; - - dev_warn(connector->dev->dev, - "%s: EDID is invalid:\n", - connector->name); - for (i = 0; i < num_blocks; i++) { - u8 *block = edid + i * EDID_LENGTH; - char prefix[20]; - - if (drm_edid_is_zero(block, EDID_LENGTH)) - sprintf(prefix, "\t[%02x] ZERO ", i); - else if (!drm_edid_block_valid(block, i, false, NULL)) - sprintf(prefix, "\t[%02x] BAD ", i); - else - sprintf(prefix, "\t[%02x] GOOD ", i); - - print_hex_dump(KERN_WARNING, - prefix, DUMP_PREFIX_NONE, 16, 1, - block, EDID_LENGTH, false); - } -} - -/* Get override or firmware EDID */ -static struct edid *drm_get_override_edid(struct drm_connector *connector) -{ - struct edid *override = NULL; - - if (connector->override_edid) - override = drm_edid_duplicate(connector->edid_blob_ptr->data); - - if (!override) - override = drm_load_edid_firmware(connector); - - return IS_ERR(override) ? NULL : override; -} - -/** - * drm_add_override_edid_modes - add modes from override/firmware EDID - * @connector: connector we're probing - * - * Add modes from the override/firmware EDID, if available. Only to be used from - * drm_helper_probe_single_connector_modes() as a fallback for when DDC probe - * failed during drm_get_edid() and caused the override/firmware EDID to be - * skipped. - * - * Return: The number of modes added or 0 if we couldn't find any. - */ -int drm_add_override_edid_modes(struct drm_connector *connector) -{ - struct edid *override; - int num_modes = 0; - - override = drm_get_override_edid(connector); - if (override) { - drm_connector_update_edid_property(connector, override); - num_modes = drm_add_edid_modes(connector, override); - kfree(override); - - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n", - connector->base.id, connector->name, num_modes); - } - - return num_modes; -} -EXPORT_SYMBOL(drm_add_override_edid_modes); - -/** - * drm_do_get_edid - get EDID data using a custom EDID block read function - * @connector: connector we're probing - * @get_edid_block: EDID block read function - * @data: private data passed to the block read function - * - * When the I2C adapter connected to the DDC bus is hidden behind a device that - * exposes a different interface to read EDID blocks this function can be used - * to get EDID data using a custom block read function. - * - * As in the general case the DDC bus is accessible by the kernel at the I2C - * level, drivers must make all reasonable efforts to expose it as an I2C - * adapter and use drm_get_edid() instead of abusing this function. - * - * The EDID may be overridden using debugfs override_edid or firmare EDID - * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority - * order. Having either of them bypasses actual EDID reads. - * - * Return: Pointer to valid EDID or NULL if we couldn't find any. - */ -struct edid *drm_do_get_edid(struct drm_connector *connector, - int (*get_edid_block)(void *data, u8 *buf, unsigned int block, - size_t len), - void *data) -{ - int i, j = 0, valid_extensions = 0; - u8 *edid, *new; - struct edid *override; - - override = drm_get_override_edid(connector); - if (override) - return override; - - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) - return NULL; - - /* base block fetch */ - for (i = 0; i < 4; i++) { - if (get_edid_block(data, edid, 0, EDID_LENGTH)) - goto out; - if (drm_edid_block_valid(edid, 0, false, - &connector->edid_corrupt)) - break; - if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { - connector->null_edid_counter++; - goto carp; - } - } - if (i == 4) - goto carp; - - /* if there's no extensions, we're done */ - valid_extensions = edid[0x7e]; - if (valid_extensions == 0) - return (struct edid *)edid; - - new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); - if (!new) - goto out; - edid = new; - - for (j = 1; j <= edid[0x7e]; j++) { - u8 *block = edid + j * EDID_LENGTH; - - for (i = 0; i < 4; i++) { - if (get_edid_block(data, block, j, EDID_LENGTH)) - goto out; - if (drm_edid_block_valid(block, j, false, NULL)) - break; - } - - if (i == 4) - valid_extensions--; - } - - if (valid_extensions != edid[0x7e]) { - u8 *base; - - connector_bad_edid(connector, edid, edid[0x7e] + 1); - - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; - edid[0x7e] = valid_extensions; - - new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, - GFP_KERNEL); - if (!new) - goto out; - - base = new; - for (i = 0; i <= edid[0x7e]; i++) { - u8 *block = edid + i * EDID_LENGTH; - - if (!drm_edid_block_valid(block, i, false, NULL)) - continue; - - memcpy(base, block, EDID_LENGTH); - base += EDID_LENGTH; - } - - kfree(edid); - edid = new; - } - - return (struct edid *)edid; - -carp: - connector_bad_edid(connector, edid, 1); -out: - kfree(edid); - return NULL; -} -EXPORT_SYMBOL_GPL(drm_do_get_edid); - -/** - * drm_probe_ddc() - probe DDC presence - * @adapter: I2C adapter to probe - * - * Return: True on success, false on failure. - */ -bool -drm_probe_ddc(struct i2c_adapter *adapter) -{ - unsigned char out; - - return (drm_do_probe_ddc_edid(adapter, &out, 0, 1) == 0); -} -EXPORT_SYMBOL(drm_probe_ddc); - -/** - * drm_get_edid - get EDID data, if available - * @connector: connector we're probing - * @adapter: I2C adapter to use for DDC - * - * Poke the given I2C channel to grab EDID data if possible. If found, - * attach it to the connector. - * - * Return: Pointer to valid EDID or NULL if we couldn't find any. - */ -struct edid *drm_get_edid(struct drm_connector *connector, - struct i2c_adapter *adapter) -{ - struct edid *edid; - - if (connector->force == DRM_FORCE_OFF) - return NULL; - - if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) - return NULL; - - edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); - if (edid) - drm_get_displayid(connector, edid); - return edid; -} -EXPORT_SYMBOL(drm_get_edid); - -/** - * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output - * @connector: connector we're probing - * @adapter: I2C adapter to use for DDC - * - * Wrapper around drm_get_edid() for laptops with dual GPUs using one set of - * outputs. The wrapper adds the requisite vga_switcheroo calls to temporarily - * switch DDC to the GPU which is retrieving EDID. - * - * Return: Pointer to valid EDID or %NULL if we couldn't find any. - */ -struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, - struct i2c_adapter *adapter) -{ - struct pci_dev *pdev = connector->dev->pdev; - struct edid *edid; - - vga_switcheroo_lock_ddc(pdev); - edid = drm_get_edid(connector, adapter); - vga_switcheroo_unlock_ddc(pdev); - - return edid; -} -EXPORT_SYMBOL(drm_get_edid_switcheroo); - /** * drm_edid_duplicate - duplicate an EDID and the extensions * @edid: EDID to duplicate @@ -5490,3 +5170,320 @@ static void drm_get_displayid(struct drm_connector *connector, connector->tile_group = NULL; } } + +#define DDC_SEGMENT_ADDR 0x30 +/** + * drm_do_probe_ddc_edid() - get EDID information via I2C + * @data: I2C device adapter + * @buf: EDID data buffer to be filled + * @block: 128 byte EDID block to start fetching from + * @len: EDID data buffer length to fetch + * + * Try to fetch EDID information by calling I2C driver functions. + * + * Return: 0 on success or -1 on failure. + */ +static int +drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len) +{ + struct i2c_adapter *adapter = data; + unsigned char start = block * EDID_LENGTH; + unsigned char segment = block >> 1; + unsigned char xfers = segment ? 3 : 2; + int ret, retries = 5; + + /* + * The core I2C driver will automatically retry the transfer if the + * adapter reports EAGAIN. However, we find that bit-banging transfers + * are susceptible to errors under a heavily loaded machine and + * generate spurious NAKs and timeouts. Retrying the transfer + * of the individual block a few times seems to overcome this. + */ + do { + struct i2c_msg msgs[] = { + { + .addr = DDC_SEGMENT_ADDR, + .flags = 0, + .len = 1, + .buf = &segment, + }, { + .addr = DDC_ADDR, + .flags = 0, + .len = 1, + .buf = &start, + }, { + .addr = DDC_ADDR, + .flags = I2C_M_RD, + .len = len, + .buf = buf, + } + }; + + /* + * Avoid sending the segment addr to not upset non-compliant + * DDC monitors. + */ + ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers); + + if (ret == -ENXIO) { + DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", + adapter->name); + break; + } + } while (ret != xfers && --retries); + + return ret == xfers ? 0 : -1; +} + +static void connector_bad_edid(struct drm_connector *connector, + u8 *edid, int num_blocks) +{ + int i; + + if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) + return; + + dev_warn(connector->dev->dev, + "%s: EDID is invalid:\n", + connector->name); + for (i = 0; i < num_blocks; i++) { + u8 *block = edid + i * EDID_LENGTH; + char prefix[20]; + + if (drm_edid_is_zero(block, EDID_LENGTH)) + sprintf(prefix, "\t[%02x] ZERO ", i); + else if (!drm_edid_block_valid(block, i, false, NULL)) + sprintf(prefix, "\t[%02x] BAD ", i); + else + sprintf(prefix, "\t[%02x] GOOD ", i); + + print_hex_dump(KERN_WARNING, + prefix, DUMP_PREFIX_NONE, 16, 1, + block, EDID_LENGTH, false); + } +} + +/* Get override or firmware EDID */ +static struct edid *drm_get_override_edid(struct drm_connector *connector) +{ + struct edid *override = NULL; + + if (connector->override_edid) + override = drm_edid_duplicate(connector->edid_blob_ptr->data); + + if (!override) + override = drm_load_edid_firmware(connector); + + return IS_ERR(override) ? NULL : override; +} + +/** + * drm_add_override_edid_modes - add modes from override/firmware EDID + * @connector: connector we're probing + * + * Add modes from the override/firmware EDID, if available. Only to be used from + * drm_helper_probe_single_connector_modes() as a fallback for when DDC probe + * failed during drm_get_edid() and caused the override/firmware EDID to be + * skipped. + * + * Return: The number of modes added or 0 if we couldn't find any. + */ +int drm_add_override_edid_modes(struct drm_connector *connector) +{ + struct edid *override; + int num_modes = 0; + + override = drm_get_override_edid(connector); + if (override) { + drm_connector_update_edid_property(connector, override); + num_modes = drm_add_edid_modes(connector, override); + kfree(override); + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n", + connector->base.id, connector->name, num_modes); + } + + return num_modes; +} +EXPORT_SYMBOL(drm_add_override_edid_modes); + +/** + * drm_do_get_edid - get EDID data using a custom EDID block read function + * @connector: connector we're probing + * @get_edid_block: EDID block read function + * @data: private data passed to the block read function + * + * When the I2C adapter connected to the DDC bus is hidden behind a device that + * exposes a different interface to read EDID blocks this function can be used + * to get EDID data using a custom block read function. + * + * As in the general case the DDC bus is accessible by the kernel at the I2C + * level, drivers must make all reasonable efforts to expose it as an I2C + * adapter and use drm_get_edid() instead of abusing this function. + * + * The EDID may be overridden using debugfs override_edid or firmware EDID + * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority + * order. Having either of them bypasses actual EDID reads. + * + * Return: Pointer to valid EDID or NULL if we couldn't find any. + */ +struct edid *drm_do_get_edid(struct drm_connector *connector, + int (*get_edid_block)(void *data, u8 *buf, unsigned int block, + size_t len), + void *data) +{ + int i, j = 0, valid_extensions = 0; + u8 *edid, *new; + struct edid *override; + + override = drm_get_override_edid(connector); + if (override) + return override; + + if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) + return NULL; + + /* base block fetch */ + for (i = 0; i < 4; i++) { + if (get_edid_block(data, edid, 0, EDID_LENGTH)) + goto out; + if (drm_edid_block_valid(edid, 0, false, + &connector->edid_corrupt)) + break; + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { + connector->null_edid_counter++; + goto carp; + } + } + if (i == 4) + goto carp; + + /* if there's no extensions, we're done */ + valid_extensions = edid[0x7e]; + if (valid_extensions == 0) + return (struct edid *)edid; + + new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); + if (!new) + goto out; + edid = new; + + for (j = 1; j <= edid[0x7e]; j++) { + u8 *block = edid + j * EDID_LENGTH; + + for (i = 0; i < 4; i++) { + if (get_edid_block(data, block, j, EDID_LENGTH)) + goto out; + if (drm_edid_block_valid(block, j, false, NULL)) + break; + } + + if (i == 4) + valid_extensions--; + } + + if (valid_extensions != edid[0x7e]) { + u8 *base; + + connector_bad_edid(connector, edid, edid[0x7e] + 1); + + edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; + edid[0x7e] = valid_extensions; + + new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, + GFP_KERNEL); + if (!new) + goto out; + + base = new; + for (i = 0; i <= edid[0x7e]; i++) { + u8 *block = edid + i * EDID_LENGTH; + + if (!drm_edid_block_valid(block, i, false, NULL)) + continue; + + memcpy(base, block, EDID_LENGTH); + base += EDID_LENGTH; + } + + kfree(edid); + edid = new; + } + + return (struct edid *)edid; + +carp: + connector_bad_edid(connector, edid, 1); +out: + kfree(edid); + return NULL; +} +EXPORT_SYMBOL_GPL(drm_do_get_edid); + +/** + * drm_probe_ddc() - probe DDC presence + * @adapter: I2C adapter to probe + * + * Return: True on success, false on failure. + */ +bool +drm_probe_ddc(struct i2c_adapter *adapter) +{ + unsigned char out; + + return (drm_do_probe_ddc_edid(adapter, &out, 0, 1) == 0); +} +EXPORT_SYMBOL(drm_probe_ddc); + +/** + * drm_get_edid - get EDID data, if available + * @connector: connector we're probing + * @adapter: I2C adapter to use for DDC + * + * Poke the given I2C channel to grab EDID data if possible. If found, + * attach it to the connector. + * + * Return: Pointer to valid EDID or NULL if we couldn't find any. + */ +struct edid *drm_get_edid(struct drm_connector *connector, + struct i2c_adapter *adapter) +{ + struct edid *edid; + + if (connector->force == DRM_FORCE_OFF) + return NULL; + + if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) + return NULL; + + edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); + if (edid) + drm_get_displayid(connector, edid); + return edid; +} +EXPORT_SYMBOL(drm_get_edid); + +/** + * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output + * @connector: connector we're probing + * @adapter: I2C adapter to use for DDC + * + * Wrapper around drm_get_edid() for laptops with dual GPUs using one set of + * outputs. The wrapper adds the requisite vga_switcheroo calls to temporarily + * switch DDC to the GPU which is retrieving EDID. + * + * Return: Pointer to valid EDID or %NULL if we couldn't find any. + */ +struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, + struct i2c_adapter *adapter) +{ + struct pci_dev *pdev = connector->dev->pdev; + struct edid *edid; + + vga_switcheroo_lock_ddc(pdev); + edid = drm_get_edid(connector, adapter); + vga_switcheroo_unlock_ddc(pdev); + + return edid; +} +EXPORT_SYMBOL(drm_get_edid_switcheroo);
The static function that populates the drm_connector tile-related fields from EDID displayid will be used outside of drm_edid.c. As it logically belongs to connector code, move it to drm_connector.c and rename it from drm_get_displayid() to drm_connector_set_displayid().
While at it, fix a few checkpatch warnings in the moved code.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_connector.c | 137 +++++++++++++++++++++++++- drivers/gpu/drm/drm_dp_mst_topology.c | 3 +- drivers/gpu/drm/drm_edid.c | 131 +----------------------- include/drm/drm_connector.h | 3 +- include/drm/drm_displayid.h | 2 + include/drm/drm_edid.h | 2 + 6 files changed, 148 insertions(+), 130 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 4c766624b20d..04f72ad0d9b6 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -21,6 +21,7 @@ */
#include <drm/drm_connector.h> +#include <drm/drm_displayid.h> #include <drm/drm_edid.h> #include <drm/drm_encoder.h> #include <drm/drm_utils.h> @@ -1801,6 +1802,135 @@ int drm_connector_set_path_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_connector_set_path_property);
+static int drm_parse_tiled_block(struct drm_connector *connector, + struct displayid_block *block) +{ + struct displayid_tiled_block *tile = + (struct displayid_tiled_block *)block; + struct drm_tile_group *tg; + u8 tile_v_loc, tile_h_loc; + u8 num_v_tile, num_h_tile; + u16 w, h; + + w = tile->tile_size[0] | tile->tile_size[1] << 8; + h = tile->tile_size[2] | tile->tile_size[3] << 8; + + num_v_tile = (tile->topo[0] & 0xf) | (tile->topo[2] & 0x30); + num_h_tile = (tile->topo[0] >> 4) | ((tile->topo[2] >> 2) & 0x30); + tile_v_loc = (tile->topo[1] & 0xf) | ((tile->topo[2] & 0x3) << 4); + tile_h_loc = (tile->topo[1] >> 4) | (((tile->topo[2] >> 2) & 0x3) << 4); + + connector->has_tile = true; + if (tile->tile_cap & 0x80) + connector->tile_is_single_monitor = true; + + connector->num_h_tile = num_h_tile + 1; + connector->num_v_tile = num_v_tile + 1; + connector->tile_h_loc = tile_h_loc; + connector->tile_v_loc = tile_v_loc; + connector->tile_h_size = w + 1; + connector->tile_v_size = h + 1; + + DRM_DEBUG_KMS("tile cap 0x%x\n", tile->tile_cap); + DRM_DEBUG_KMS("tile_size %d x %d\n", w + 1, h + 1); + DRM_DEBUG_KMS("topo num tiles %dx%d, location %dx%d\n", + num_h_tile + 1, num_v_tile + 1, tile_h_loc, tile_v_loc); + DRM_DEBUG_KMS("vend %c%c%c\n", tile->topology_id[0], + tile->topology_id[1], tile->topology_id[2]); + + tg = drm_mode_get_tile_group(connector->dev, tile->topology_id); + if (!tg) + tg = drm_mode_create_tile_group(connector->dev, + tile->topology_id); + if (!tg) + return -ENOMEM; + + if (connector->tile_group != tg) { + /* + * If we haven't got a pointer, take the reference, drop ref to + * old tile group. + */ + if (connector->tile_group) + drm_mode_put_tile_group(connector->dev, + connector->tile_group); + connector->tile_group = tg; + } else { + /* If same tile group, then release the ref we just took. */ + drm_mode_put_tile_group(connector->dev, tg); + } + + return 0; +} + +static int drm_connector_parse_displayid_tile(struct drm_connector *connector, + u8 *displayid, int length) +{ + struct displayid_block *block; + int idx; + int ret; + + if (!drm_displayid_is_valid(displayid, length)) + return -EINVAL; + + idx = sizeof(struct displayid_hdr); + for_each_displayid_db(displayid, block, idx, length) { + DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n", + block->tag, block->rev, block->num_bytes); + + switch (block->tag) { + case DATA_BLOCK_TILED_DISPLAY: + ret = drm_parse_tiled_block(connector, block); + if (ret) + return ret; + break; + case DATA_BLOCK_TYPE_1_DETAILED_TIMING: + /* handled in mode gathering code. */ + break; + case DATA_BLOCK_CTA: + /* handled in the cea parser code. */ + break; + default: + DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", + block->tag); + break; + } + } + return 0; +} + +static void drm_connector_set_displayid_tile(struct drm_connector *connector, + void *displayid, + unsigned int length) +{ + connector->has_tile = false; + + if (displayid) + drm_connector_parse_displayid_tile(connector, displayid, + length); + + if (!connector->has_tile && connector->tile_group) { + drm_mode_put_tile_group(connector->dev, connector->tile_group); + connector->tile_group = NULL; + } +} + +static void drm_connector_set_edid_tile(struct drm_connector *connector, + const struct edid *edid) +{ + unsigned int length; + void *displayid; + + if (edid) { + displayid = drm_edid_find_displayid_extension(edid); + length = EDID_LENGTH - 1; + } else { + displayid = NULL; + length = 0; + } + + drm_connector_set_displayid_tile(connector, displayid, length); +} + /** * drm_connector_set_tile_property - set tile property on connector * @connector: connector to set property on. @@ -1814,12 +1944,15 @@ EXPORT_SYMBOL(drm_connector_set_path_property); * Returns: * Zero on success, errno on failure. */ -int drm_connector_set_tile_property(struct drm_connector *connector) +int drm_connector_set_tile_property(struct drm_connector *connector, + const struct edid *edid) { struct drm_device *dev = connector->dev; char tile[256]; int ret;
+ drm_connector_set_edid_tile(connector, edid); + if (!connector->has_tile) { ret = drm_property_replace_global_blob(dev, &connector->tile_blob_ptr, @@ -1899,7 +2032,7 @@ int drm_connector_update_edid_property(struct drm_connector *connector, dev->mode_config.edid_property); if (ret) return ret; - return drm_connector_set_tile_property(connector); + return drm_connector_set_tile_property(connector, edid); } EXPORT_SYMBOL(drm_connector_update_edid_property);
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 82add736e17d..6edf9bf8fd9b 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1712,7 +1712,8 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, port->port_num >= DP_MST_LOGICAL_PORT_0) { port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); - drm_connector_set_tile_property(port->connector); + drm_connector_set_tile_property(port->connector, + port->cached_edid); } (*mstb->mgr->cbs->register_connector)(port->connector); } diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index bfcb232b9760..541e50b858e3 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2606,7 +2606,7 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id) return edid_ext; }
-static bool drm_displayid_is_valid(u8 *displayid, unsigned int length) +bool drm_displayid_is_valid(u8 *displayid, unsigned int length) { struct displayid_hdr *base; u8 csum = 0; @@ -2631,7 +2631,7 @@ static bool drm_displayid_is_valid(u8 *displayid, unsigned int length) return true; }
-static u8 *drm_find_displayid_extension(const struct edid *edid) +u8 *drm_edid_find_displayid_extension(const struct edid *edid) { u8 *ext = drm_find_edid_extension(edid, DISPLAYID_EXT);
@@ -2659,7 +2659,7 @@ static u8 *drm_find_cea_extension(const struct edid *edid) return cea;
/* CEA blocks can also be found embedded in a DisplayID block */ - displayid = drm_find_displayid_extension(edid); + displayid = drm_edid_find_displayid_extension(edid); if (!displayid) return NULL;
@@ -4501,7 +4501,7 @@ static int add_displayid_detailed_modes(struct drm_connector *connector, struct displayid_block *block; int num_modes = 0;
- displayid = drm_find_displayid_extension(edid); + displayid = drm_edid_find_displayid_extension(edid); if (!displayid) return 0;
@@ -5055,122 +5055,6 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame, } EXPORT_SYMBOL(drm_hdmi_vendor_infoframe_from_display_mode);
-static int drm_parse_tiled_block(struct drm_connector *connector, - struct displayid_block *block) -{ - struct displayid_tiled_block *tile = (struct displayid_tiled_block *)block; - u16 w, h; - u8 tile_v_loc, tile_h_loc; - u8 num_v_tile, num_h_tile; - struct drm_tile_group *tg; - - w = tile->tile_size[0] | tile->tile_size[1] << 8; - h = tile->tile_size[2] | tile->tile_size[3] << 8; - - num_v_tile = (tile->topo[0] & 0xf) | (tile->topo[2] & 0x30); - num_h_tile = (tile->topo[0] >> 4) | ((tile->topo[2] >> 2) & 0x30); - tile_v_loc = (tile->topo[1] & 0xf) | ((tile->topo[2] & 0x3) << 4); - tile_h_loc = (tile->topo[1] >> 4) | (((tile->topo[2] >> 2) & 0x3) << 4); - - connector->has_tile = true; - if (tile->tile_cap & 0x80) - connector->tile_is_single_monitor = true; - - connector->num_h_tile = num_h_tile + 1; - connector->num_v_tile = num_v_tile + 1; - connector->tile_h_loc = tile_h_loc; - connector->tile_v_loc = tile_v_loc; - connector->tile_h_size = w + 1; - connector->tile_v_size = h + 1; - - DRM_DEBUG_KMS("tile cap 0x%x\n", tile->tile_cap); - DRM_DEBUG_KMS("tile_size %d x %d\n", w + 1, h + 1); - DRM_DEBUG_KMS("topo num tiles %dx%d, location %dx%d\n", - num_h_tile + 1, num_v_tile + 1, tile_h_loc, tile_v_loc); - DRM_DEBUG_KMS("vend %c%c%c\n", tile->topology_id[0], tile->topology_id[1], tile->topology_id[2]); - - tg = drm_mode_get_tile_group(connector->dev, tile->topology_id); - if (!tg) { - tg = drm_mode_create_tile_group(connector->dev, tile->topology_id); - } - if (!tg) - return -ENOMEM; - - if (connector->tile_group != tg) { - /* if we haven't got a pointer, - take the reference, drop ref to old tile group */ - if (connector->tile_group) { - drm_mode_put_tile_group(connector->dev, connector->tile_group); - } - connector->tile_group = tg; - } else - /* if same tile group, then release the ref we just took. */ - drm_mode_put_tile_group(connector->dev, tg); - return 0; -} - -static int drm_parse_displayid(struct drm_connector *connector, - u8 *displayid, int length) -{ - struct displayid_block *block; - int idx; - int ret; - - if (!drm_displayid_is_valid(displayid, length)) - return -EINVAL; - - idx = sizeof(struct displayid_hdr); - for_each_displayid_db(displayid, block, idx, length) { - DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n", - block->tag, block->rev, block->num_bytes); - - switch (block->tag) { - case DATA_BLOCK_TILED_DISPLAY: - ret = drm_parse_tiled_block(connector, block); - if (ret) - return ret; - break; - case DATA_BLOCK_TYPE_1_DETAILED_TIMING: - /* handled in mode gathering code. */ - break; - case DATA_BLOCK_CTA: - /* handled in the cea parser code. */ - break; - default: - DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag); - break; - } - } - return 0; -} - -static void drm_get_displayid(struct drm_connector *connector, - struct edid *edid) -{ - void *displayid; - int ret; - - connector->has_tile = false; - displayid = drm_find_displayid_extension(edid); - if (!displayid) { - /* drop reference to any tile group we had */ - goto out_drop_ref; - } - - ret = drm_parse_displayid(connector, displayid, EDID_LENGTH - 1); - if (ret < 0) - goto out_drop_ref; - if (!connector->has_tile) - goto out_drop_ref; - return; - -out_drop_ref: - if (connector->tile_group) { - drm_mode_put_tile_group(connector->dev, connector->tile_group); - connector->tile_group = NULL; - } -} - #define DDC_SEGMENT_ADDR 0x30 /** * drm_do_probe_ddc_edid() - get EDID information via I2C @@ -5448,18 +5332,13 @@ EXPORT_SYMBOL(drm_probe_ddc); struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { - struct edid *edid; - if (connector->force == DRM_FORCE_OFF) return NULL;
if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) return NULL;
- edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); - if (edid) - drm_get_displayid(connector, edid); - return edid; + return drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); } EXPORT_SYMBOL(drm_get_edid);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 681cb590f952..9ff07f8d6a44 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1532,7 +1532,8 @@ int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
int drm_connector_set_path_property(struct drm_connector *connector, const char *path); -int drm_connector_set_tile_property(struct drm_connector *connector); +int drm_connector_set_tile_property(struct drm_connector *connector, + const struct edid *edid); int drm_connector_update_edid_property(struct drm_connector *connector, const struct edid *edid); void drm_connector_set_link_status_property(struct drm_connector *connector, diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h index 9d3b745c3107..24f2faf3abf8 100644 --- a/include/drm/drm_displayid.h +++ b/include/drm/drm_displayid.h @@ -100,4 +100,6 @@ struct displayid_detailed_timing_block { (idx) += (block)->num_bytes + sizeof(struct displayid_block), \ (block) = (struct displayid_block *)&(displayid)[idx])
+bool drm_displayid_is_valid(u8 *displayid, unsigned int length); + #endif diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index b9719418c3d2..cf58e6c987d1 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -495,6 +495,8 @@ int drm_edid_header_is_valid(const u8 *raw_edid); bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *edid_corrupt); bool drm_edid_is_valid(struct edid *edid); +u8 *drm_edid_find_displayid_extension(const struct edid *edid); + void drm_edid_get_monitor_name(struct edid *edid, char *name, int buflen); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
The drm_get_edid() function skips EDID read when the connector is disabled through connector->force. drm_do_get_edid(), which is supposed to be used instead of drm_get_edid() for drivers that need to provide a custom DDC read method, is missing the connector force checks. This breaks the connector sysfs or command line force attribute.
Fix this by moving the the connector checks to drm_do_get_edid(). For convenience, the existing drm_do_get_edid() function is renamed to __drm_do_get_edid() and used in drm_do_get_edid().
While at it, remove the incorrect statement from the drm_get_edid() documentation related to attaching the read EDID to the connector.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_edid.c | 80 +++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 541e50b858e3..4b28635f1050 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5191,27 +5191,17 @@ int drm_add_override_edid_modes(struct drm_connector *connector) } EXPORT_SYMBOL(drm_add_override_edid_modes);
-/** - * drm_do_get_edid - get EDID data using a custom EDID block read function - * @connector: connector we're probing - * @get_edid_block: EDID block read function - * @data: private data passed to the block read function - * - * When the I2C adapter connected to the DDC bus is hidden behind a device that - * exposes a different interface to read EDID blocks this function can be used - * to get EDID data using a custom block read function. - * - * As in the general case the DDC bus is accessible by the kernel at the I2C - * level, drivers must make all reasonable efforts to expose it as an I2C - * adapter and use drm_get_edid() instead of abusing this function. - * - * The EDID may be overridden using debugfs override_edid or firmware EDID - * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority - * order. Having either of them bypasses actual EDID reads. - * - * Return: Pointer to valid EDID or NULL if we couldn't find any. - */ -struct edid *drm_do_get_edid(struct drm_connector *connector, +static bool __drm_probe_ddc(int (*get_edid_block)(void *data, u8 *buf, + unsigned int block, + size_t len), + void *data) +{ + unsigned char out; + + return (get_edid_block(data, &out, 0, 1) == 0); +} + +static struct edid *__drm_do_get_edid(struct drm_connector *connector, int (*get_edid_block)(void *data, u8 *buf, unsigned int block, size_t len), void *data) @@ -5302,6 +5292,41 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, kfree(edid); return NULL; } + +/** + * drm_do_get_edid - get EDID data using a custom EDID block read function + * @connector: connector we're probing + * @get_edid_block: EDID block read function + * @data: private data passed to the block read function + * + * When the I2C adapter connected to the DDC bus is hidden behind a device that + * exposes a different interface to read EDID blocks this function can be used + * to get EDID data using a custom block read function. + * + * As in the general case the DDC bus is accessible by the kernel at the I2C + * level, drivers must make all reasonable efforts to expose it as an I2C + * adapter and use drm_get_edid() instead of abusing this function. + * + * The EDID may be overridden using debugfs override_edid or firmware EDID + * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority + * order. Having either of them bypasses actual EDID reads. + * + * Return: Pointer to valid EDID or NULL if we couldn't find any. + */ +struct edid *drm_do_get_edid(struct drm_connector *connector, + int (*get_edid_block)(void *data, u8 *buf, unsigned int block, + size_t len), + void *data) +{ + if (connector->force == DRM_FORCE_OFF) + return NULL; + + if (connector->force == DRM_FORCE_UNSPECIFIED && + !__drm_probe_ddc(get_edid_block, data)) + return NULL; + + return __drm_do_get_edid(connector, get_edid_block, data); +} EXPORT_SYMBOL_GPL(drm_do_get_edid);
/** @@ -5313,9 +5338,7 @@ EXPORT_SYMBOL_GPL(drm_do_get_edid); bool drm_probe_ddc(struct i2c_adapter *adapter) { - unsigned char out; - - return (drm_do_probe_ddc_edid(adapter, &out, 0, 1) == 0); + return __drm_probe_ddc(drm_do_probe_ddc_edid, adapter); } EXPORT_SYMBOL(drm_probe_ddc);
@@ -5324,20 +5347,13 @@ EXPORT_SYMBOL(drm_probe_ddc); * @connector: connector we're probing * @adapter: I2C adapter to use for DDC * - * Poke the given I2C channel to grab EDID data if possible. If found, - * attach it to the connector. + * Poke the given I2C channel to grab EDID data if possible. * * Return: Pointer to valid EDID or NULL if we couldn't find any. */ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { - if (connector->force == DRM_FORCE_OFF) - return NULL; - - if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) - return NULL; - return drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); } EXPORT_SYMBOL(drm_get_edid);
This patch is a proof of concept showing how EDID retrieval could be decoupled from drm_connector, in order to avoid passing the connector pointer to the drm_bridge .get_edid() operation. The user of such bridges (in most case the future drm_bridge_connector helper, see "[PATCH v2 00/50] drm/omap: Replace custom display drivers with drm_bridge and drm_panel") would need to duplicate the logic found in drm_do_get_edid(), and bridges would use the __drm_do_get_edid() function to retrieve and return a drm_edid structure.
Not-yet-signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_edid.c | 157 ++++++++++++++++++++++--------------- 1 file changed, 95 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 4b28635f1050..37b6a6de9f42 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -5119,34 +5119,6 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len) return ret == xfers ? 0 : -1; }
-static void connector_bad_edid(struct drm_connector *connector, - u8 *edid, int num_blocks) -{ - int i; - - if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) - return; - - dev_warn(connector->dev->dev, - "%s: EDID is invalid:\n", - connector->name); - for (i = 0; i < num_blocks; i++) { - u8 *block = edid + i * EDID_LENGTH; - char prefix[20]; - - if (drm_edid_is_zero(block, EDID_LENGTH)) - sprintf(prefix, "\t[%02x] ZERO ", i); - else if (!drm_edid_block_valid(block, i, false, NULL)) - sprintf(prefix, "\t[%02x] BAD ", i); - else - sprintf(prefix, "\t[%02x] GOOD ", i); - - print_hex_dump(KERN_WARNING, - prefix, DUMP_PREFIX_NONE, 16, 1, - block, EDID_LENGTH, false); - } -} - /* Get override or firmware EDID */ static struct edid *drm_get_override_edid(struct drm_connector *connector) { @@ -5201,31 +5173,67 @@ static bool __drm_probe_ddc(int (*get_edid_block)(void *data, u8 *buf, return (get_edid_block(data, &out, 0, 1) == 0); }
-static struct edid *__drm_do_get_edid(struct drm_connector *connector, +struct drm_edid { + u8 *data; + unsigned int num_blocks; + + unsigned int null_counter; + unsigned int bad_counter; + bool corrupt; +}; + +static void connector_bad_edid(struct drm_device *dev, const char *name, + struct drm_edid *edid) +{ + int i; + + if (edid->bad_counter++ && !(drm_debug & DRM_UT_KMS)) + return; + + dev_warn(dev->dev, "%s: EDID is invalid:\n", name); + + for (i = 0; i < edid->num_blocks; i++) { + u8 *block = edid->data + i * EDID_LENGTH; + char prefix[20]; + + if (drm_edid_is_zero(block, EDID_LENGTH)) + sprintf(prefix, "\t[%02x] ZERO ", i); + else if (!drm_edid_block_valid(block, i, false, NULL)) + sprintf(prefix, "\t[%02x] BAD ", i); + else + sprintf(prefix, "\t[%02x] GOOD ", i); + + print_hex_dump(KERN_WARNING, + prefix, DUMP_PREFIX_NONE, 16, 1, + block, EDID_LENGTH, false); + } +} + +static int __drm_do_get_edid(struct drm_device *dev, const char *name, + struct drm_edid *edid, int (*get_edid_block)(void *data, u8 *buf, unsigned int block, size_t len), void *data) { - int i, j = 0, valid_extensions = 0; - u8 *edid, *new; - struct edid *override; + unsigned int valid_extensions; + unsigned int i, j; + u8 *new; + int ret;
- override = drm_get_override_edid(connector); - if (override) - return override; + edid->data = kmalloc(EDID_LENGTH, GFP_KERNEL); + if (!edid->data) + return -ENOMEM;
- if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) - return NULL; + edid->num_blocks = 1;
/* base block fetch */ for (i = 0; i < 4; i++) { - if (get_edid_block(data, edid, 0, EDID_LENGTH)) + if (get_edid_block(data, edid->data, 0, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(edid, 0, false, - &connector->edid_corrupt)) + if (drm_edid_block_valid(edid->data, 0, false, &edid->corrupt)) break; - if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { - connector->null_edid_counter++; + if (i == 0 && drm_edid_is_zero(edid->data, EDID_LENGTH)) { + edid->null_counter++; goto carp; } } @@ -5233,17 +5241,19 @@ static struct edid *__drm_do_get_edid(struct drm_connector *connector, goto carp;
/* if there's no extensions, we're done */ - valid_extensions = edid[0x7e]; + valid_extensions = edid->data[0x7e] + 1; if (valid_extensions == 0) - return (struct edid *)edid; + return 0;
- new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); + edid->num_blocks += valid_extensions; + + new = krealloc(edid->data, edid->num_blocks * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; - edid = new; + edid->data = new;
- for (j = 1; j <= edid[0x7e]; j++) { - u8 *block = edid + j * EDID_LENGTH; + for (j = 1; j < edid->num_blocks; j++) { + u8 *block = edid->data + j * EDID_LENGTH;
for (i = 0; i < 4; i++) { if (get_edid_block(data, block, j, EDID_LENGTH)) @@ -5256,22 +5266,25 @@ static struct edid *__drm_do_get_edid(struct drm_connector *connector, valid_extensions--; }
- if (valid_extensions != edid[0x7e]) { + if (valid_extensions != edid->num_blocks - 1) { u8 *base;
- connector_bad_edid(connector, edid, edid[0x7e] + 1); + connector_bad_edid(dev, name, edid);
- edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions; - edid[0x7e] = valid_extensions; + edid->data[EDID_LENGTH-1] += edid->data[0x7e] - valid_extensions; + edid->data[0x7e] = valid_extensions; + edid->num_blocks = valid_extensions + 1;
new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, GFP_KERNEL); - if (!new) + if (!new) { + ret = -ENOMEM; goto out; + }
base = new; - for (i = 0; i <= edid[0x7e]; i++) { - u8 *block = edid + i * EDID_LENGTH; + for (i = 0; i < edid->num_blocks; i++) { + u8 *block = edid->data + i * EDID_LENGTH;
if (!drm_edid_block_valid(block, i, false, NULL)) continue; @@ -5280,17 +5293,19 @@ static struct edid *__drm_do_get_edid(struct drm_connector *connector, base += EDID_LENGTH; }
- kfree(edid); - edid = new; + kfree(edid->data); + edid->data = new; }
- return (struct edid *)edid; + return 0;
carp: - connector_bad_edid(connector, edid, 1); + connector_bad_edid(dev, name, edid); + ret = -EINVAL; out: - kfree(edid); - return NULL; + kfree(edid->data); + edid->data = NULL; + return ret; }
/** @@ -5318,6 +5333,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, size_t len), void *data) { + struct drm_edid edid; + struct edid *override; + if (connector->force == DRM_FORCE_OFF) return NULL;
@@ -5325,7 +5343,22 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, !__drm_probe_ddc(get_edid_block, data)) return NULL;
- return __drm_do_get_edid(connector, get_edid_block, data); + override = drm_get_override_edid(connector); + if (override) + return override; + + memset(&edid, 0, sizeof(edid)); + edid.bad_counter = connector->bad_edid_counter; + edid.null_counter = connector->null_edid_counter; + + __drm_do_get_edid(connector->dev, connector->name, &edid, + get_edid_block, data); + + connector->bad_edid_counter = edid.bad_counter; + connector->null_edid_counter = edid.null_counter; + connector->edid_corrupt = edid.corrupt; + + return (struct edid *)edid.data; } EXPORT_SYMBOL_GPL(drm_do_get_edid);
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Ville Syrjälä