Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Tomasz Figa <tfiga@chromium.org>]

On Fri, May 22, 2020 at 5:05 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> On Fri, 2020-05-22 at 16:15 +0200, Dafna Hirschfeld wrote:
> >
> > On 22.05.20 15:31, Ezequiel Garcia wrote:
> > > Hi Dafna, Helen,
> > >
> > > On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote:
> > > > On 21.05.20 00:08, Helen Koike wrote:
> > > > > On 5/20/20 6:54 PM, Helen Koike wrote:
> > > > > > Hi Dafna,
> > > > > >
> > > > > > On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> > > > > > > The resize block of rkisp1 always get the input as yuv422.
> > > > > > > Instead of defining the default hdiv, vdiv as macros, the
> > > > > > > code is more clear if it takes the (hv)div from the YUV422P
> > > > > > > info. This makes it clear where those values come from.
> > > > > > > The patch also adds documentation to explain that.
> > > > > > >
> > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > > > >
> > > > > > Acked-by: Helen Koike <helen.koike@collabora.com>
> > > > > >
> > > > > > Thanks!
> > > > > > Helen
> > > > > >
> > > > > > > ---
> > > > > > >    drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
> > > > > > >    1 file changed, 12 insertions(+), 13 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > > > > index d049374413dc..04a29af8cc92 100644
> > > > > > > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > > > > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > > > > @@ -16,9 +16,6 @@
> > > > > > >    #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8
> > > > > > >    #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV
> > > > > > >
> > > > > > > -#define RKISP1_MBUS_FMT_HDIV 2
> > > > > > > -#define RKISP1_MBUS_FMT_VDIV 1
> > > > > > > -
> > > > > > >    enum rkisp1_shadow_regs_when {
> > > > > > >     RKISP1_SHADOW_REGS_SYNC,
> > > > > > >     RKISP1_SHADOW_REGS_ASYNC,
> > > > > > > @@ -361,11 +358,12 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz,
> > > > > > >    static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
> > > > > > >                           enum rkisp1_shadow_regs_when when)
> > > > > > >    {
> > > > > > > -   u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV;
> > > > > > >     struct v4l2_rect sink_y, sink_c, src_y, src_c;
> > > > > > >     struct v4l2_mbus_framefmt *src_fmt;
> > > > > > >     struct v4l2_rect *sink_crop;
> > > > > > >     struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> > > > > > > +   const struct v4l2_format_info *yuv422_info =
> > > > > > > +           v4l2_format_info(V4L2_PIX_FMT_YUV422P);
> > > > > > >
> > >
> > > Instead of hardcoding this fourcc, is there any way we can
> > > retrieve it from a configured format?
> > >
> > What do you mean?
> > If the configured format is bayer then the resizer is disabled.
> > Otherwise the resizer always get the input as yuv422, this is why it is hard coded.
> >
>
> I don't like to rely on these assumptions/knowledge.
> It's much cleaner to retrieve the format, and avoiding
> hardcoding things as much as you can.

It would indeed be cleaner if we could retrieve this format from
somewhere, but where would that be? In theory we could assign a
YUV4:2:2 mbus format to the resizer input pad, but I think there is no
similar data available for mbs formats, is there?

Actually, if we look at this a bit more strictly, V4L2_PIX_FMT_YUV422P
is not exactly what the resizer gets at its input.
V4L2_PIX_FMT_YUV422P is a specific memory representation and the
corresponding v4l2_format_info struct contains data about the memory
layout. The resizer gets an unspecified YUV 4:2:2 pixel stream. Making
the code suggest that it's V4L2_PIX_FMT_YUV422P might make it more
confusing in another way.

Perhaps the way forward would be to simply add a comment explaining
where the 2 and 1 dividers come from?

Best regards,
Tomasz