[PATCH 1/4] drm/edid: move displayid tiled block parsing into separate function.

[PATCH 1/4] drm/edid: move displayid tiled block parsing into separate function.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This just makes the code easier to follow.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_edid.c | 111 ++++++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 96b181a..9dbaaf4 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4152,6 +4152,60 @@ 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_display_id(struct drm_connector *connector,
 				u8 *displayid, int length,
 				bool is_edid_extension)
@@ -4162,6 +4216,7 @@ static int drm_parse_display_id(struct drm_connector *connector,
 	struct displayid_block *block;
 	u8 csum = 0;
 	int i;
+	int ret;
 
 	if (is_edid_extension)
 		idx = 1;
@@ -4187,58 +4242,10 @@ static int drm_parse_display_id(struct drm_connector *connector,
 		      block->tag, block->rev, block->num_bytes);
 
 	switch (block->tag) {
-	case DATA_BLOCK_TILED_DISPLAY: {
-		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);
-	}
-		break;
+	case DATA_BLOCK_TILED_DISPLAY:
+		ret = drm_parse_tiled_block(connector, block);
+		if (ret)
+			return ret;
 	default:
 		printk("unknown displayid tag %d\n", block->tag);
 		break;
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/displayid: Iterate over all DisplayID blocks

[Dave Airlie]

From: Tomas Bzatek <tomas@bzatek.net>

This will iterate over all DisplayID blocks found in the buffer.
Previously only the first block was parsed.

https://bugs.freedesktop.org/show_bug.cgi?id=95207

Signed-off-by: Tomas Bzatek <tomas@bzatek.net>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_edid.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9dbaaf4..3cf17a3 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4237,18 +4237,24 @@ static int drm_parse_display_id(struct drm_connector *connector,
 		return -EINVAL;
 	}
 
-	block = (struct displayid_block *)&displayid[idx + 4];
-	DRM_DEBUG_KMS("block id %d, 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;
-	default:
-		printk("unknown displayid tag %d\n", block->tag);
-		break;
+	idx += sizeof(struct displayid_hdr);
+	while (block = (struct displayid_block *)&displayid[idx],
+	       idx + sizeof(struct displayid_block) <= length &&
+	       idx + sizeof(struct displayid_block) + block->num_bytes <= length &&
+	       block->num_bytes > 0) {
+		idx += block->num_bytes + sizeof(struct displayid_block);
+		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;
+		default:
+			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
+			break;
+		}
 	}
 	return 0;
 }
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/edid: move displayid validation to it's own function.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

We need to use this for validating modeline additions.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_edid.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3cf17a3..73d4218 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3901,6 +3901,29 @@ static void drm_add_display_info(struct edid *edid,
 		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
 }
 
+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_ERROR("DisplayID checksum invalid, remainder is %d\n", csum);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 /**
  * drm_add_edid_modes - add modes from EDID data, if available
  * @connector: connector we're probing
@@ -4212,30 +4235,15 @@ static int drm_parse_display_id(struct drm_connector *connector,
 {
 	/* if this is an EDID extension the first byte will be 0x70 */
 	int idx = 0;
-	struct displayid_hdr *base;
 	struct displayid_block *block;
-	u8 csum = 0;
-	int i;
 	int ret;
 
 	if (is_edid_extension)
 		idx = 1;
 
-	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_ERROR("DisplayID checksum invalid, remainder is %d\n", csum);
-		return -EINVAL;
-	}
+	ret = validate_displayid(displayid, length, idx);
+	if (ret)
+		return ret;
 
 	idx += sizeof(struct displayid_hdr);
 	while (block = (struct displayid_block *)&displayid[idx],
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/edid: add displayid detailed 1 timings to the modelist. (v1.1)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

The tiled 5K Dell monitor appears to be hiding it's tiled mode
inside the displayid timings block, this patch parses this
blocks and adds the modes to the modelist.

v1.1: add missing __packed.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95207
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_edid.c  | 108 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_displayid.h |  17 +++++++
 2 files changed, 125 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 73d4218..46ccd60 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3924,6 +3924,110 @@ static int validate_displayid(u8 *displayid, int length, int idx)
 	return 0;
 }
 
+static struct drm_display_mode *drm_mode_displayid_detailed(struct drm_device *dev,
+							    struct displayid_detailed_timings_1 *timings)
+{
+	struct drm_display_mode *mode;
+	unsigned pixel_clock = (timings->pixel_clock[0] |
+				(timings->pixel_clock[1] << 8) |
+				(timings->pixel_clock[2] << 16));
+	unsigned hactive = (timings->hactive[0] | timings->hactive[1] << 8) + 1;
+	unsigned hblank = (timings->hblank[0] | timings->hblank[1] << 8) + 1;
+	unsigned hsync = (timings->hsync[0] | (timings->hsync[1] & 0x7f) << 8) + 1;
+	unsigned hsync_width = (timings->hsw[0] | timings->hsw[1] << 8) + 1;
+	unsigned vactive = (timings->vactive[0] | timings->vactive[1] << 8) + 1;
+	unsigned vblank = (timings->vblank[0] | timings->vblank[1] << 8) + 1;
+	unsigned vsync = (timings->vsync[0] | (timings->vsync[1] & 0x7f) << 8) + 1;
+	unsigned vsync_width = (timings->vsw[0] | timings->vsw[1] << 8) + 1;
+	bool hsync_positive = (timings->hsync[1] >> 7) & 0x1;
+	bool vsync_positive = (timings->vsync[1] >> 7) & 0x1;
+	mode = drm_mode_create(dev);
+	if (!mode)
+		return NULL;
+
+	mode->clock = pixel_clock * 10;
+	mode->hdisplay = hactive;
+	mode->hsync_start = mode->hdisplay + hsync;
+	mode->hsync_end = mode->hsync_start + hsync_width;
+	mode->htotal = mode->hdisplay + hblank;
+
+	mode->vdisplay = vactive;
+	mode->vsync_start = mode->vdisplay + vsync;
+	mode->vsync_end = mode->vsync_start + vsync_width;
+	mode->vtotal = mode->vdisplay + vblank;
+
+	mode->flags = 0;
+	mode->flags |= hsync_positive ? DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC;
+	mode->flags |= vsync_positive ? DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC;
+	mode->type = DRM_MODE_TYPE_DRIVER;
+
+	if (timings->flags & 0x80)
+		mode->type |= DRM_MODE_TYPE_PREFERRED;
+	mode->vrefresh = drm_mode_vrefresh(mode);
+	drm_mode_set_name(mode);
+
+	return mode;
+}
+
+static int add_displayid_detailed_1_modes(struct drm_connector *connector,
+					  struct displayid_block *block)
+{
+	struct displayid_detailed_timing_block *det = (struct displayid_detailed_timing_block *)block;
+	int i;
+	int num_timings;
+	struct drm_display_mode *newmode;
+	int num_modes = 0;
+	/* blocks must be multiple of 20 bytes length */
+	if (block->num_bytes % 20)
+		return 0;
+
+	num_timings = block->num_bytes / 20;
+	for (i = 0; i < num_timings; i++) {
+		struct displayid_detailed_timings_1 *timings = &det->timings[i];
+
+		newmode = drm_mode_displayid_detailed(connector->dev, timings);
+		if (!newmode)
+			continue;
+
+		drm_mode_probed_add(connector, newmode);
+		num_modes++;
+	}
+	return num_modes;
+}
+
+static int add_displayid_detailed_modes(struct drm_connector *connector,
+					struct edid *edid)
+{
+	u8 *displayid;
+	int ret;
+	int idx = 1;
+	int length = EDID_LENGTH;
+	struct displayid_block *block;
+	int num_modes = 0;
+
+	displayid = drm_find_displayid_extension(edid);
+	if (!displayid)
+		return 0;
+
+	ret = validate_displayid(displayid, length, idx);
+	if (ret)
+		return 0;
+
+	idx += sizeof(struct displayid_hdr);
+	while (block = (struct displayid_block *)&displayid[idx],
+	       idx + sizeof(struct displayid_block) <= length &&
+	       idx + sizeof(struct displayid_block) + block->num_bytes <= length &&
+	       block->num_bytes > 0) {
+		idx += block->num_bytes + sizeof(struct displayid_block);
+		switch (block->tag) {
+		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
+			num_modes += add_displayid_detailed_1_modes(connector, block);
+			break;
+		}
+	}
+	return num_modes;
+}
+
 /**
  * drm_add_edid_modes - add modes from EDID data, if available
  * @connector: connector we're probing
@@ -3969,6 +4073,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 	num_modes += add_established_modes(connector, edid);
 	num_modes += add_cea_modes(connector, edid);
 	num_modes += add_alternate_cea_modes(connector, edid);
+	num_modes += add_displayid_detailed_modes(connector, edid);
 	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
 		num_modes += add_inferred_modes(connector, edid);
 
@@ -4259,6 +4364,9 @@ static int drm_parse_display_id(struct drm_connector *connector,
 			ret = drm_parse_tiled_block(connector, block);
 			if (ret)
 				return ret;
+		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
+			/* handled in mode gathering code. */
+			break;
 		default:
 			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
 			break;
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 623b4e9..c0d4df6 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -73,4 +73,21 @@ struct displayid_tiled_block {
 	u8 topology_id[8];
 } __packed;
 
+struct displayid_detailed_timings_1 {
+	u8 pixel_clock[3];
+	u8 flags;
+	u8 hactive[2];
+	u8 hblank[2];
+	u8 hsync[2];
+	u8 hsw[2];
+	u8 vactive[2];
+	u8 vblank[2];
+	u8 vsync[2];
+	u8 vsw[2];
+} __packed;
+
+struct displayid_detailed_timing_block {
+	struct displayid_block base;
+	struct displayid_detailed_timings_1 timings[0];
+};
 #endif
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/edid: move displayid tiled block parsing into separate function.

[Jani Nikula]

On Tue, 10 May 2016, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This just makes the code easier to follow.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 111 ++++++++++++++++++++++++---------------------
>  1 file changed, 59 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 96b181a..9dbaaf4 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4152,6 +4152,60 @@ 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_display_id(struct drm_connector *connector,
>  				u8 *displayid, int length,
>  				bool is_edid_extension)
> @@ -4162,6 +4216,7 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  	struct displayid_block *block;
>  	u8 csum = 0;
>  	int i;
> +	int ret;
>  
>  	if (is_edid_extension)
>  		idx = 1;
> @@ -4187,58 +4242,10 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  		      block->tag, block->rev, block->num_bytes);
>  
>  	switch (block->tag) {
> -	case DATA_BLOCK_TILED_DISPLAY: {
> -		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);
> -	}
> -		break;
> +	case DATA_BLOCK_TILED_DISPLAY:
> +		ret = drm_parse_tiled_block(connector, block);
> +		if (ret)
> +			return ret;
>  	default:
>  		printk("unknown displayid tag %d\n", block->tag);
>  		break;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/edid: move displayid tiled block parsing into separate function.

[Jani Nikula]

On Tue, 10 May 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 10 May 2016, Dave Airlie <airlied@gmail.com> wrote:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> This just makes the code easier to follow.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Ooops, one thing needs fixing below.

>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 111 ++++++++++++++++++++++++---------------------
>>  1 file changed, 59 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 96b181a..9dbaaf4 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4152,6 +4152,60 @@ 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_display_id(struct drm_connector *connector,
>>  				u8 *displayid, int length,
>>  				bool is_edid_extension)
>> @@ -4162,6 +4216,7 @@ static int drm_parse_display_id(struct drm_connector *connector,
>>  	struct displayid_block *block;
>>  	u8 csum = 0;
>>  	int i;
>> +	int ret;
>>  
>>  	if (is_edid_extension)
>>  		idx = 1;
>> @@ -4187,58 +4242,10 @@ static int drm_parse_display_id(struct drm_connector *connector,
>>  		      block->tag, block->rev, block->num_bytes);
>>  
>>  	switch (block->tag) {
>> -	case DATA_BLOCK_TILED_DISPLAY: {
>> -		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);
>> -	}
>> -		break;
>> +	case DATA_BLOCK_TILED_DISPLAY:
>> +		ret = drm_parse_tiled_block(connector, block);
>> +		if (ret)
>> +			return ret;

break; here.

>>  	default:
>>  		printk("unknown displayid tag %d\n", block->tag);
>>  		break;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/displayid: Iterate over all DisplayID blocks

[Jani Nikula]

On Tue, 10 May 2016, Dave Airlie <airlied@gmail.com> wrote:
> From: Tomas Bzatek <tomas@bzatek.net>
>
> This will iterate over all DisplayID blocks found in the buffer.
> Previously only the first block was parsed.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=95207
>
> Signed-off-by: Tomas Bzatek <tomas@bzatek.net>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9dbaaf4..3cf17a3 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4237,18 +4237,24 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  		return -EINVAL;
>  	}
>  
> -	block = (struct displayid_block *)&displayid[idx + 4];
> -	DRM_DEBUG_KMS("block id %d, 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;
> -	default:
> -		printk("unknown displayid tag %d\n", block->tag);
> -		break;
> +	idx += sizeof(struct displayid_hdr);
> +	while (block = (struct displayid_block *)&displayid[idx],
> +	       idx + sizeof(struct displayid_block) <= length &&
> +	       idx + sizeof(struct displayid_block) + block->num_bytes <= length &&
> +	       block->num_bytes > 0) {

Ugh, that's a bit ugly, but seems to DTRT.

Do we need to check block->num_bytes > 0? Shouldn't that be block->tag
specific? See below.

> +		idx += block->num_bytes + sizeof(struct displayid_block);
> +		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:

This could use a check (in a separate patch, perhaps) to verify idx +
sizeof(struct displayid_tiled_block) <= length. (Alternatively,
sizeof(struct displayid_block) + block->num_bytes == sizeof(struct
displayid_tiled_block).)

In any case,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +			ret = drm_parse_tiled_block(connector, block);
> +			if (ret)
> +				return ret;
> +		default:
> +			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
> +			break;
> +		}
>  	}
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/edid: move displayid validation to it's own function.

[Jani Nikula]

On Tue, 10 May 2016, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> We need to use this for validating modeline additions.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3cf17a3..73d4218 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3901,6 +3901,29 @@ static void drm_add_display_info(struct edid *edid,
>  		info->color_formats |= DRM_COLOR_FORMAT_YCRCB422;
>  }
>  
> +static int validate_displayid(u8 *displayid, int length, int idx)

Bikeshed, (u8 *displayid, int idx, int length) would feel like a more
natural order to me.

> +{
> +	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);
> +

I guess to be pedantic we should check idx + sizeof(struct
displayid_hdr) <= length before looking at base. This patch is about
abstracting the thing, so should be a separate patch anyway.

Other than the bikeshed,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



> +	if (base->bytes + 5 > length - idx)
> +		return -EINVAL;
> +	for (i = idx; i <= base->bytes + 5; i++) {
> +		csum += displayid[i];
> +	}
> +	if (csum) {
> +		DRM_ERROR("DisplayID checksum invalid, remainder is %d\n", csum);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * drm_add_edid_modes - add modes from EDID data, if available
>   * @connector: connector we're probing
> @@ -4212,30 +4235,15 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  {
>  	/* if this is an EDID extension the first byte will be 0x70 */
>  	int idx = 0;
> -	struct displayid_hdr *base;
>  	struct displayid_block *block;
> -	u8 csum = 0;
> -	int i;
>  	int ret;
>  
>  	if (is_edid_extension)
>  		idx = 1;
>  
> -	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_ERROR("DisplayID checksum invalid, remainder is %d\n", csum);
> -		return -EINVAL;
> -	}
> +	ret = validate_displayid(displayid, length, idx);
> +	if (ret)
> +		return ret;
>  
>  	idx += sizeof(struct displayid_hdr);
>  	while (block = (struct displayid_block *)&displayid[idx],

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel