Re: [PATCH 08/12] drm: Set the relevant infoframe field when scanning out a 3D mode

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Damien Lespiau <damien.lespiau@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 08/12] drm: Set the relevant infoframe field when scanning out a 3D mode
Date: Mon, 16 Sep 2013 21:59:19 +0300	[thread overview]
Message-ID: <20130916185919.GN4531@intel.com> (raw)
In-Reply-To: <1379353735-4472-9-git-send-email-damien.lespiau@intel.com>

On Mon, Sep 16, 2013 at 06:48:51PM +0100, Damien Lespiau wrote:
> When scanning out a 3D mode on HDMI, we need to send an HDMI infoframe
> with the corresponding layout to the sink.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e016a5d..9a36b6e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3364,6 +3364,18 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  }
>  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>  
> +static enum hdmi_3d_structure
> +s3d_structure_from_display_mode(const struct drm_display_mode *mode)
> +{
> +	u32 s3d_mode = (mode->flags & DRM_MODE_FLAG_3D_MASK) >> 14;
> +	int set = ffs(s3d_mode) - 1;
> +
> +	if (set == 7)
> +		return HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF;

This function feels a bit too subtle. I would perhaps go w/ just a
switch statement. Or maybe leave a hole for 7 in the DRM_MODE_FLAG_3D
flags. And if we run out of bits before 3d_structure=7 gets defined we
just repurpose the bit for something else. But maybe that's equally
subtle.

Hmm. The spec is quite poor too. In one place it says the quincunx modes
are valid for 3d_structure=8 (and 15 is reserved), but in another place it
says 3d_structure=15 is when the quincunx stuff applies. But I guss we
can just keep ignoring the 3d_structure > 8 case for now.

> +
> +	return set;
> +}
> +
>  /**
>   * drm_hdmi_vendor_infoframe_from_display_mode() - fill an HDMI infoframe with
>   * data from a DRM display mode
> @@ -3381,20 +3393,29 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  					    const struct drm_display_mode *mode)
>  {
>  	int err;
> +	u32 s3d_flags;
>  	u8 vic;
>  
>  	if (!frame || !mode)
>  		return -EINVAL;
>  
>  	vic = drm_match_hdmi_mode(mode);
> -	if (!vic)
> +	s3d_flags = mode->flags & DRM_MODE_FLAG_3D_MASK;
> +
> +	if (!vic && !s3d_flags)
> +		return -EINVAL;
> +
> +	if (vic && s3d_flags)
>  		return -EINVAL;

Could be just '!vic == !s3d_flags' or w/ !! on both sides if we want
to stick to positive thinking.

But maybe it's me who's getting into the subtle territory here :)

Other than the bikesheds it looks OK to me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	err = hdmi_vendor_infoframe_init(frame);
>  	if (err < 0)
>  		return err;
>  
> -	frame->vic = vic;
> +	if (vic)
> +		frame->vic = vic;
> +	else
> +		frame->s3d_struct = s3d_structure_from_display_mode(mode);
>  
>  	return 0;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC