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