Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 8/9] media: dwc: dw-hdmi-rx: Add support for Aspect Ratio
To:     Nelson Costa <Nelson.Costa@synopsys.com>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Vinod Koul <vkoul@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Jose Abreu <Jose.Abreu@synopsys.com>
References: <cover.1622631488.git.nelson.costa@synopsys.com>
 <79c8b378a0b9d73e8116fe31a1ebb06fcc6b0925.1622631488.git.nelson.costa@synopsys.com>
From:   Hans Verkuil <hverkuil-cisco@xs4all.nl>
Message-ID: <0004bb3f-13dc-b376-d491-f4023c716aa7@xs4all.nl>
Date:   Wed, 2 Jun 2021 14:34:53 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.10.0
MIME-Version: 1.0
In-Reply-To: <79c8b378a0b9d73e8116fe31a1ebb06fcc6b0925.1622631488.git.nelson.costa@synopsys.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 02/06/2021 13:24, Nelson Costa wrote:
> This adds support to get aspect ratio for the current
> video being received and provide the info through v4l2
> API query_dv_timings.
> 
> Signed-off-by: Nelson Costa <nelson.costa@synopsys.com>
> ---
>  drivers/media/platform/dwc/dw-hdmi-rx.c | 54 +++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/dwc/dw-hdmi-rx.c b/drivers/media/platform/dwc/dw-hdmi-rx.c
> index b20eccc..a468a93 100644
> --- a/drivers/media/platform/dwc/dw-hdmi-rx.c
> +++ b/drivers/media/platform/dwc/dw-hdmi-rx.c
> @@ -2250,13 +2250,31 @@ static u32 dw_hdmi_get_width(struct dw_hdmi_dev *dw_dev)
>  	return width;
>  }
>  
> +static int dw_hdmi_vic_to_cea861(u8 hdmi_vic)
> +{
> +	switch (hdmi_vic) {
> +	case 1:
> +		return 95;
> +	case 2:
> +		return 94;
> +	case 3:
> +		return 93;
> +	case 4:
> +		return 98;
> +	default:
> +		return 0;
> +	}
> +}

This should be in v4l2-dv-timings.c. It's a useful generic function.

> +
>  static int dw_hdmi_query_dv_timings(struct v4l2_subdev *sd,
>  				    struct v4l2_dv_timings *timings)
>  {
>  	struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
>  	struct v4l2_bt_timings *bt = &timings->bt;
> +	struct v4l2_dv_timings t = {0};

Use '= {}', it looks a bit nicer that way.

>  	bool is_hdmi_vic;
>  	u32 htot, hofs;
> +	u8 cea861_vic;
>  	u32 vtot;
>  	u8 vic;
>  
> @@ -2351,8 +2369,40 @@ static int dw_hdmi_query_dv_timings(struct v4l2_subdev *sd,
>  		}
>  	}
>  
> -	dev_dbg(dw_dev->dev, "%s: width=%u, height=%u, mbuscode=%u\n", __func__,
> -		bt->width, bt->height, dw_hdmi_get_mbus_code(dw_dev));
> +	if (is_hdmi_vic)
> +		cea861_vic = dw_hdmi_vic_to_cea861(bt->hdmi_vic);
> +	else
> +		cea861_vic = vic;

This definitely is needed, but note that this is unrelated to the Aspect Ratio
support. This should be done in a separate patch.

> +
> +	/* picture aspect ratio based on v4l2 dv timings array */
> +	if (v4l2_find_dv_timings_cea861_vic(&t, cea861_vic)) {
> +		/* when the numerator/denominator are zero means that the
> +		 * picture aspect ratio is the same of the active measures ratio
> +		 */
> +		if (!t.bt.picture_aspect.numerator) {
> +			unsigned long n, d;
> +
> +			rational_best_approximation(t.bt.width, t.bt.height,
> +						    t.bt.width, t.bt.height,
> +						    &n, &d);
> +			t.bt.picture_aspect.numerator = n;
> +			t.bt.picture_aspect.denominator = d;
> +		}
> +
> +		bt->picture_aspect = t.bt.picture_aspect;

Why do this? picture_aspect is only used if it is non-square (V4L2_DV_FL_HAS_PICTURE_ASPECT
is set), so there is no need to set it if V4L2_DV_FL_HAS_PICTURE_ASPECT is cleared.

I don't see any reason for this.

Regards,

	Hans

> +	} else {
> +		bt->picture_aspect.numerator = 0;
> +		bt->picture_aspect.denominator = 0;
> +		dev_dbg(dw_dev->dev,
> +			"%s: cea861_vic=%d was not found in v4l2 dv timings",
> +			__func__, cea861_vic);
> +	}
> +
> +	dev_dbg(dw_dev->dev,
> +		"%s: width=%u, height=%u, mbuscode=%u, cea861_vic=%d, ar={%d,%d}\n",
> +		__func__, bt->width, bt->height, dw_hdmi_get_mbus_code(dw_dev),
> +		cea861_vic, bt->picture_aspect.numerator,
> +		bt->picture_aspect.denominator);
>  
>  	return 0;
>  }
>