linux-kernel - RE: [PATCH 8/9] media: dwc: dw-hdmi-rx: Add support for Aspect Ratio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <MW3PR12MB4459286DD0105FC25230753AC13D9@MW3PR12MB4459.namprd12.prod.outlook.com>
Date:   Wed, 2 Jun 2021 17:35:49 +0000
From:   Nelson Costa <Nelson.Costa@...opsys.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Jose Abreu <Jose.Abreu@...opsys.com>
Subject: RE: [PATCH 8/9] media: dwc: dw-hdmi-rx: Add support for Aspect Ratio

Hi Hans,

Thanks for your comments and feedback!

From: Hans Verkuil <hverkuil-cisco@...all.nl>
Date: qua, jun 02, 2021 at 13:34:53

> 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@...opsys.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.
> 

I agree! I will make a specific patch for this in the next patch series.

> > +
> >  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.
> 

I agree!

> >  	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.
> 

Ok.

> > +
> > +	/* 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.
> 

I agree! That related part will be removed then, once for the square vics 
it's
normal to have both "numerator == denominator == 0" and user space 
applications 
shall handle properly with that.

Thanks,

BR,

Nelson Costa

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