Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers

[Lars-Peter Clausen <lars@metafoo.de>]

On 12/05/2012 05:45 PM, Thierry Reding wrote:
> Add a generic helper to fill in an HDMI AVI infoframe with data
> extracted from a DRM display mode.

That's a very nice patch series, comes in pretty handy. Thanks :)

I've just one comment.

> [...]
> +static inline enum hdmi_picture_aspect
> +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
> +{
> +	enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
> +
> +	if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
> +		aspect = HDMI_PICTURE_ASPECT_16_9;
> +	else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
> +		aspect = HDMI_PICTURE_ASPECT_4_3;
> +
> +	return aspect;
> +}
> +
> +/**
> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
> + *                                              data from a DRM display mode
> + * @frame: HDMI AVI infoframe
> + * @mode: DRM display mode
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int
> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> +					 const struct drm_display_mode *mode)
> +{
[...]
> +
> +	frame->picture_aspect = drm_display_mode_get_aspect(mode);

As far as I know the picture aspect ratio is supposed to be the aspect ratio
of the displayed content not the pixel aspect ratio. The receiver already
knows the pixel aspect ratio since it knows both the pixel width and the
height. The picture aspect ratio is something that could be set by a
userspace application running in fullscreen, e.g. a video player.

> +	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> +
> +	return 0;
> +}