From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
Date: Wed, 05 Dec 2012 18:51:20 +0100
Message-ID: <50BF8998.6060207@metafoo.de>
References: <1354725944-1862-1-git-send-email-thierry.reding@avionic-design.de>
	<1354725944-1862-4-git-send-email-thierry.reding@avionic-design.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org>
Received: from smtp-out-241.synserver.de (smtp-out-241.synserver.de
	[212.40.185.241])
	by gabe.freedesktop.org (Postfix) with ESMTP id 9356CE5F42
	for <dri-devel@lists.freedesktop.org>;
	Wed,  5 Dec 2012 10:07:25 -0800 (PST)
In-Reply-To: <1354725944-1862-4-git-send-email-thierry.reding@avionic-design.de>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/dri-devel>,
	<mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/dri-devel>,
	<mailto:dri-devel-request@lists.freedesktop.org?subject=subscribe>
Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org
Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org
To: Thierry Reding <thierry.reding@avionic-design.de>
Cc: dri-devel@lists.freedesktop.org
List-Id: dri-devel@lists.freedesktop.org

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;
> +}