Re: [PATCH 08/20] drm: set output colorspace in AVI infoframe

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

On Mon, Jul 10, 2017 at 04:48:36PM +0530, Shashank Sharma wrote:
> A source must set output colorspace information in AVI
> infoframes, so that the sink can decode upcoming frames
> accordingly.
> 
> This patch adds a function to add the output colorspace
> information in the AVI infoframes.
> 
> V2: Rebase
> V3: Rebase
> V4: Rebase
> V5: Rebase
> V6: Made patch independent of HDMI output type.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 29 +++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  5 +++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 944a28f..cede86e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4796,6 +4796,35 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
>  
>  /**
> + * drm_hdmi_avi_infoframe_set_colorspace - fill an HDMI AVI infoframe with
> + * colorspace data of the output type
> + *
> + * @frame: HDMI AVI infoframe
> + * @mode: DRM display mode
> + * @hdmi_output: HDMI output colorspace
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int
> +drm_hdmi_avi_infoframe_set_colorspace(struct hdmi_avi_infoframe *frame,
> +				      const struct drm_display_mode *mode,
> +				      enum hdmi_colorspace colorspace)
> +{
> +	if (colorspace > HDMI_COLORSPACE_YUV420 ||
> +		colorspace < HDMI_COLORSPACE_RGB) {
> +		DRM_ERROR("Invalid color space type\n");
> +		return -EINVAL;
> +	}

Seems overly defensive. I'd say that if someone insists on writing
buggy code just let them do it.

> +
> +	frame->colorspace = colorspace;
> +	if (colorspace == HDMI_COLORSPACE_YUV420)
> +		frame->pixel_repeat = 0;

Most VICs don't allow pixel repeat in 444/etc. either, and we don't
protect against that. So this looks like pretty pointless check in
this form.

So IMO just drop this entire patch and just assign frame->colorspace in
the driver.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_hdmi_avi_infoframe_set_colorspace);
> +
> +/**
>   * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
>   *                                        quantization range information
>   * @frame: HDMI AVI infoframe
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index aa58146..b79e0cb 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -332,6 +332,7 @@ struct cea_sad {
>  struct drm_encoder;
>  struct drm_connector;
>  struct drm_display_mode;
> +enum drm_hdmi_output_type;
>  
>  void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid);
>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
> @@ -354,6 +355,10 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  					 const struct drm_display_mode *mode,
>  					 bool is_hdmi2_sink);
>  int
> +drm_hdmi_avi_infoframe_set_colorspace(struct hdmi_avi_infoframe *frame,
> +					 const struct drm_display_mode *mode,
> +					 enum hdmi_colorspace colorspace);
> +int
>  drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  					    const struct drm_display_mode *mode);
>  void
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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