RE: [PATCH 7/9] media: v4l2-dv-timings: Add more CEA/CTA-861 video format timings

[Nelson Costa <Nelson.Costa@synopsys.com>]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Date: ter, jun 15, 2021 at 09:39:51

> On 14/06/2021 19:04, Nelson Costa wrote:
> > Hi Hans,
> > 
> > Thanks for your comments!
> > 
> > From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Date: qua, jun 02, 2021 at 19:19:25
> > 
> >> On 02/06/2021 19:15, Nelson Costa wrote:
> >>> Hi Hans,
> >>>
> >>> Thanks for your comments and feedback!
> >>>
> >>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>> Date: qua, jun 02, 2021 at 13:26:17
> >>>
> >>>> Hi Nelson,
> >>>>
> >>>> On 02/06/2021 13:24, Nelson Costa wrote:
> >>>>> This extends the support for more video format timings based
> >>>>> on SPECs CEA-861-F and CTA-861-G.
> >>>>>
> >>>>> NOTE: For the newer SPECs the CEA was unified to the CTA.
> >>>>> The CTA-861-G then includes the CEA-861-F timings besides
> >>>>> the new timings that are specified.
> >>>>>
> >>>>> CEA-861-F: Specifies the Video timings for VICs 1-107.
> >>>>> CTA-861-G: Specifies the Video timings for VICs 1-107, 108-127, 193-219.
> >>>>>
> >>>>> With this patch, the array v4l2_dv_timings_presets has support for
> >>>>> all video timings specified in CTA-861-G.
> >>>>>
> >>>>> Signed-off-by: Nelson Costa <nelson.costa@synopsys.com>
> >>>>> ---
> >>>>>  drivers/media/v4l2-core/v4l2-dv-timings.c |  139 +++
> >>>>>  include/uapi/linux/v4l2-dv-timings.h      | 1595 ++++++++++++++++++++++++++++-
> >>>>
> >>>> I prefer to split this up in two patches, one for each header.
> >>>>
> >>>
> >>> I agree! It will be addressed in the next patch series.
> >>>
> >>>> The v4l2-dv-timings.h changes look good (my compliments for all the
> >>>> work you put into that!).
> >>>>
> >>>
> >>> Thanks!
> >>>
> >>>> I am more concerned about adding all these timings to v4l2_dv_timings_presets.
> >>>>
> >>>> There are really two different things going on here: the v4l2_dv_timings_presets
> >>>> array is used both by v4l2_enum_dv_timings_cap() to list supported commonly used
> >>>> timings, or to match against timings parameters (v4l2_find_dv_timings_cap()), and
> >>>> as a lookup table when receiving a specific VIC code (v4l2_find_dv_timings_cea861_vic()).
> >>>>
> >>>> All the new timings you added are really only relevant in the last case when you
> >>>> have the vic code.
> >>>>
> >>>> I think it is better to create a second array v4l2_dv_timings_non_square_vics[]
> >>>> (or a better name!) that contains these timings.
> >>>>
> >>>
> >>> I understood.
> >>>
> >>> We can then create another array as you said. But when you say 
> >>> "non_square"
> >>> you mean that the vics have "Pixel Aspect Ratio != 1:1"?
> >>>
> >>> Because the new vics added have both kind of vics with "Pixel Aspect 
> >>> Ratio != 1:1"
> >>> and also "Pixel Aspect Ratio == 1:1".
> >>
> >> There are? It's confusing since for 1:1 pixel aspect ratios I expect that the
> >> picture aspect ratio is set to { 0, 0 }, instead they are all filled in.
> >>
> >> I think it will be clearer if I see a v2 where the picture aspect ratio and
> >> the V4L2_DV_FL_HAS_PICTURE_ASPECT flag are only set for the non-square pixel
> >> timings. Also, for the timings with 1:1 pixel aspect ratio you don't need to
> >> add the PA... suffix. That suffix only makes sense for non-square pixel aspect
> >> ratios. It's confusing otherwise.
> >>
> > 
> > It makes sense! That way it will assure coherence with the current 
> > implementation.
> > In the v2 patch series this will be addressed.
> > 
> >>>
> >>> So, for the new vics should we create a second array with name 
> >>> v4l2_dv_timings_extended_vics[]?
> >>
> >> The new vics with non-square pixel aspect ratios, or with pixel repetition.
> >>
> > 
> > You mean the new vics added that are square should be kept in the 
> > original array?
> > 
> > And only the new vics that are non-square or with pixel repetition should 
> > go to a second array?
> 
> Correct.
> 
> Regards,
> 
> 	Hans

Thanks a lot for your feedback! :)
It will be addressed in the v2.

BR,

Nelson Costa