Re: [PATCH 2/2] drm/edid: parse the list of additional 3D modes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 28, 2013 at 05:12:47PM +0000, Thomas Wood wrote:
> Parse 2D_VIC_order_X and 3D_Structure_X from the list at the end of the
> HDMI Vendor Specific Data Block.
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1dd82cd..eb6b363 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2808,6 +2808,56 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  						     video_len, i);
>  	}
>  
> +	if (hdmi_3d_len <= 4)

This number 4 should depend on the value of multi_present, no?

Also we haven't actually verified that all hdmi_3d_len bytes
fit within the data block. If you do that a bit earlier, you
could also skip the explicit 'len' checks for the
multi_present cases since we also check those against
hdmi_3d_len.

> +		goto out;
> +
> +	offset += 4;
> +
> +	for (i = 0; i < (hdmi_3d_len - 4); i++) {
> +		int vic_index;
> +		struct drm_display_mode *newmode = NULL;
> +		unsigned int newflag = 0;
> +
> +		if (((db[8 + offset + i] & 0x0f) > 7)
> +		    && (i + 1 == hdmi_3d_len - 4))
> +			break;

The '(db[8 + offset + i] & 0x0f) > 7' part is repeated a few times.
Maybe add a 'bool detail_present = (db[8 + offset + i] & 0x0f) > 7' or
something to make make things a bit easier to read.

> +
> +		/* 2D_VIC_order_X */
> +		vic_index = db[8 + offset + i] >> 4;
> +
> +		/* 3D_Structure_X */
> +		switch (db[8 + offset + i] & 0x0f) {
> +		case 0:
> +			newflag = DRM_MODE_FLAG_3D_FRAME_PACKING;
> +			break;
> +		case 6:
> +			newflag = DRM_MODE_FLAG_3D_TOP_AND_BOTTOM;
> +			break;
> +		case 8:

Maybe add a /* 3D_Detail_X */ comment here
for consistency with the other comments.

> +			if ((db[9 + offset + i] >> 4) == 1)
> +				newflag = DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF;
> +			break;
> +		}
> +
> +		if (newflag != 0) {
> +			newmode = drm_display_mode_from_vic_index(connector,
> +								  video_db,
> +								  video_len,
> +								  vic_index);
> +
> +			if (newmode) {
> +				newmode->flags |= newflag;
> +				drm_mode_probed_add(connector, newmode);
> +				modes++;
> +			}
> +		}
> +
> +		if ((db[8 + offset + i] & 0x0f) > 7) {
> +			/* Optional 3D_Detail_X and reserved */
> +			i++;
> +		}
> +	}
> +
>  out:
>  	return modes;
>  }
> -- 
> 1.8.4.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC