[rfc] drm/edid: add support for displayid timings

[rfc] drm/edid: add support for displayid timings

[Dave Airlie]

It appears that Dell 5K monitors hide some mode timings
in the displayid block, this set of patches fixes up a few
bits of displayid support, then add support for extracting
the type 1 detailed timings that the Dell monitors use.

The first two patches are missing signoff but I've asked
for Tomas to provide them.

This at least gets the modelines into the modelist.

Dave.

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

[PATCH 1/5] drm/displayid: Enhance version reporting

[Dave Airlie]

From: Tomas Bzatek <tomas@bzatek.net>

Cosmetic change, let's report more precise revisions and IDs.

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

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_edid.c  | 6 +++---
 include/drm/drm_displayid.h | 6 ++++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 9a9be9a..c8a3a55 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4168,8 +4168,8 @@ static int drm_parse_display_id(struct drm_connector *connector,
 
 	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);
+	DRM_DEBUG_KMS("base revision v%d.%d, edid length %d, bytes %d, prod_id %d ext_count %d\n",
+		      base->ver, base->rev, length, base->bytes, base->prod_id, base->ext_count);
 
 	if (base->bytes + 5 > length - idx)
 		return -EINVAL;
@@ -4183,7 +4183,7 @@ static int drm_parse_display_id(struct drm_connector *connector,
 	}
 
 	block = (struct displayid_block *)&displayid[idx + 4];
-	DRM_DEBUG_KMS("block id %d, rev %d, len %d\n",
+	DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
 		      block->tag, block->rev, block->num_bytes);
 
 	switch (block->tag) {
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 623b4e9..042f9fc 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -52,7 +52,8 @@
 #define PRODUCT_TYPE_DIRECT_DRIVE 6
 
 struct displayid_hdr {
-	u8 rev;
+	u8 rev:4;
+	u8 ver:4;
 	u8 bytes;
 	u8 prod_id;
 	u8 ext_count;
@@ -60,7 +61,8 @@ struct displayid_hdr {
 
 struct displayid_block {
 	u8 tag;
-	u8 rev;
+	u8 rev:3;
+	u8 reserved:5;
 	u8 num_bytes;
 } __packed;
 
-- 
2.5.5

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

[PATCH 2/5] 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: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_edid.c | 124 ++++++++++++++++++++++++---------------------
 1 file changed, 65 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c8a3a55..1a364b3 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4182,66 +4182,72 @@ static int drm_parse_display_id(struct drm_connector *connector,
 		return -EINVAL;
 	}
 
-	block = (struct displayid_block *)&displayid[idx + 4];
-	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: {
-		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);
+	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: {
+			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);
 			}
-			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;
-	default:
-		printk("unknown displayid tag %d\n", block->tag);
-		break;
+			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;
+		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/5] 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 | 110 ++++++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1a364b3..e4c681f 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;
@@ -4192,57 +4247,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);
-		}
+		case DATA_BLOCK_TILED_DISPLAY:
+			ret = drm_parse_tiled_block(connector, block);
+			if (ret)
+				return ret;
 			break;
 		default:
 			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
-- 
2.5.5

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

[PATCH 4/5] 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 | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e4c681f..e85d828 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3901,6 +3901,30 @@ 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 v%d.%d, edid length %d, bytes %d, prod_id %d ext_count %d\n",
+		      base->ver, base->rev, length, 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 +4236,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 v%d.%d, edid length %d, bytes %d, prod_id %d ext_count %d\n",
-		      base->ver, base->rev, length, 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 5/5] drm/edid: add displayid detailed 1 timings to the modelist.

[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.

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e85d828..aca9e25 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3925,6 +3925,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
@@ -3970,6 +4074,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);
 
diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 042f9fc..edef51d 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -75,4 +75,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];
+};
+
+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 3/5] drm/edid: move displayid tiled block parsing into separate function.

[Jani Nikula]

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

Swapping the order of patches 2 and 3 would make the patch series easier
to follow. ;)

BR,
Jani.

>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 110 ++++++++++++++++++++++++---------------------
>  1 file changed, 59 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1a364b3..e4c681f 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;
> @@ -4192,57 +4247,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);
> -		}
> +		case DATA_BLOCK_TILED_DISPLAY:
> +			ret = drm_parse_tiled_block(connector, block);
> +			if (ret)
> +				return ret;
>  			break;
>  		default:
>  			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);

-- 
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 5/5] drm/edid: add displayid detailed 1 timings to the modelist.

[Jani Nikula]

On Tue, 03 May 2016, Dave Airlie <airlied@gmail.com> wrote:
> 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.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95207
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 105 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_displayid.h |  17 +++++++
>  2 files changed, 122 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e85d828..aca9e25 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3925,6 +3925,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
> @@ -3970,6 +4074,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);
>  
> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
> index 042f9fc..edef51d 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -75,4 +75,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];

An alternative would be to declare these fields as __le16, and you could
read them in drm_mode_displayid_detailed() using le16_to_cpu().

Anyway, these structs should be __packed.

BR,
Jani.


> +};
> +
> +struct displayid_detailed_timing_block {
> +	struct displayid_block base;
> +	struct displayid_detailed_timings_1 timings[0];
> +};
>  #endif

-- 
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/5] drm/displayid: Enhance version reporting

[Ville Syrjälä]

On Wed, May 04, 2016 at 06:36:48AM +1000, Dave Airlie wrote:
> From: Tomas Bzatek <tomas@bzatek.net>
> 
> Cosmetic change, let's report more precise revisions and IDs.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=95207
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 6 +++---
>  include/drm/drm_displayid.h | 6 ++++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9a9be9a..c8a3a55 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4168,8 +4168,8 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  
>  	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);
> +	DRM_DEBUG_KMS("base revision v%d.%d, edid length %d, bytes %d, prod_id %d ext_count %d\n",
> +		      base->ver, base->rev, length, base->bytes, base->prod_id, base->ext_count);
>  
>  	if (base->bytes + 5 > length - idx)
>  		return -EINVAL;
> @@ -4183,7 +4183,7 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  	}
>  
>  	block = (struct displayid_block *)&displayid[idx + 4];
> -	DRM_DEBUG_KMS("block id %d, rev %d, len %d\n",
> +	DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
>  		      block->tag, block->rev, block->num_bytes);
>  
>  	switch (block->tag) {
> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
> index 623b4e9..042f9fc 100644
> --- a/include/drm/drm_displayid.h
> +++ b/include/drm/drm_displayid.h
> @@ -52,7 +52,8 @@
>  #define PRODUCT_TYPE_DIRECT_DRIVE 6
>  
>  struct displayid_hdr {
> -	u8 rev;
> +	u8 rev:4;
> +	u8 ver:4;
>  	u8 bytes;
>  	u8 prod_id;
>  	u8 ext_count;
> @@ -60,7 +61,8 @@ struct displayid_hdr {
>  
>  struct displayid_block {
>  	u8 tag;
> -	u8 rev;
> +	u8 rev:3;
> +	u8 reserved:5;
>  	u8 num_bytes;
>  } __packed;

Using bitfields in an architecture independent structure doesn't
feel like an entirely good idea to me.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/displayid: Enhance version reporting

[Michel Dänzer]

On 04.05.2016 18:10, Ville Syrjälä wrote:
> On Wed, May 04, 2016 at 06:36:48AM +1000, Dave Airlie wrote:
>> From: Tomas Bzatek <tomas@bzatek.net>
>>
>> Cosmetic change, let's report more precise revisions and IDs.
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=95207
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 6 +++---
>>  include/drm/drm_displayid.h | 6 ++++--
>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 9a9be9a..c8a3a55 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4168,8 +4168,8 @@ static int drm_parse_display_id(struct drm_connector *connector,
>>  
>>  	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);
>> +	DRM_DEBUG_KMS("base revision v%d.%d, edid length %d, bytes %d, prod_id %d ext_count %d\n",
>> +		      base->ver, base->rev, length, base->bytes, base->prod_id, base->ext_count);
>>  
>>  	if (base->bytes + 5 > length - idx)
>>  		return -EINVAL;
>> @@ -4183,7 +4183,7 @@ static int drm_parse_display_id(struct drm_connector *connector,
>>  	}
>>  
>>  	block = (struct displayid_block *)&displayid[idx + 4];
>> -	DRM_DEBUG_KMS("block id %d, rev %d, len %d\n",
>> +	DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
>>  		      block->tag, block->rev, block->num_bytes);
>>  
>>  	switch (block->tag) {
>> diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
>> index 623b4e9..042f9fc 100644
>> --- a/include/drm/drm_displayid.h
>> +++ b/include/drm/drm_displayid.h
>> @@ -52,7 +52,8 @@
>>  #define PRODUCT_TYPE_DIRECT_DRIVE 6
>>  
>>  struct displayid_hdr {
>> -	u8 rev;
>> +	u8 rev:4;
>> +	u8 ver:4;
>>  	u8 bytes;
>>  	u8 prod_id;
>>  	u8 ext_count;
>> @@ -60,7 +61,8 @@ struct displayid_hdr {
>>  
>>  struct displayid_block {
>>  	u8 tag;
>> -	u8 rev;
>> +	u8 rev:3;
>> +	u8 reserved:5;
>>  	u8 num_bytes;
>>  } __packed;
> 
> Using bitfields in an architecture independent structure doesn't
> feel like an entirely good idea to me.

Yeah, this won't work as expected on some architectures.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/displayid: Enhance version reporting

[Dave Airlie]

>>> index 623b4e9..042f9fc 100644
>>> --- a/include/drm/drm_displayid.h
>>> +++ b/include/drm/drm_displayid.h
>>> @@ -52,7 +52,8 @@
>>>  #define PRODUCT_TYPE_DIRECT_DRIVE 6
>>>
>>>  struct displayid_hdr {
>>> -    u8 rev;
>>> +    u8 rev:4;
>>> +    u8 ver:4;
>>>      u8 bytes;
>>>      u8 prod_id;
>>>      u8 ext_count;
>>> @@ -60,7 +61,8 @@ struct displayid_hdr {
>>>
>>>  struct displayid_block {
>>>      u8 tag;
>>> -    u8 rev;
>>> +    u8 rev:3;
>>> +    u8 reserved:5;
>>>      u8 num_bytes;
>>>  } __packed;
>>
>> Using bitfields in an architecture independent structure doesn't
>> feel like an entirely good idea to me.
>
> Yeah, this won't work as expected on some architectures.
>
Indeed, I'll drop this for now.

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

Re: [PATCH 5/5] drm/edid: add displayid detailed 1 timings to the modelist.

[Dave Airlie]

>>
>> +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];
>
> An alternative would be to declare these fields as __le16, and you could
> read them in drm_mode_displayid_detailed() using le16_to_cpu().

We don't do that for the EDID structs, so I'd rather avoid it here as well.

The spec is always in terms of 8-bit values.
>
> Anyway, these structs should be __packed.

Indeed.

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