Re: [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats

From: Tomasz Figa <tfiga@chromium.org>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Helen Koike <helen.koike@collabora.com>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	kernel@collabora.com, Dafna Hirschfeld <dafna3@gmail.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>
Subject: Re: [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats
Date: Wed, 10 Jun 2020 21:28:00 +0200	[thread overview]
Message-ID: <CAAFQd5BMHX_kTr4CWnjB=Lbdzmg+8uFEeW5qVKitMQZ8EkZf5g@mail.gmail.com> (raw)
In-Reply-To: <bb5576b1-7bfb-7911-004c-8fa1b4e1603c@collabora.com>

On Wed, Jun 10, 2020 at 9:00 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> On 10.06.20 20:36, Dafna Hirschfeld wrote:
> >
> >
> > On 10.06.20 19:15, Tomasz Figa wrote:
> >> Hi Dafna,
> >>
> >> On Tue, Jun 09, 2020 at 05:28:22PM +0200, Dafna Hirschfeld wrote:
> >>> The rkisp1_resizer's enum callback 'rkisp1_rsz_enum_mbus_code'
> >>> calls the enum callback of the 'rkisp1_isp' on it's video sink pad.
> >>> This is a bug, the resizer should support the same formats
> >>> supported by the 'rkisp1_isp' on the source pad (not the sink pad).
> >>>
> >>> Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"
> >>>
> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>> Acked-by: Helen Koike <helen.koike@collabora.com>
> >>> ---
> >>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>
> >> Thank you for the patch. Please see my comments inline.
> >>
> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> index d049374413dc..d64c064bdb1d 100644
> >>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> @@ -437,8 +437,8 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
> >>>       u32 pad = code->pad;
> >>>       int ret;
> >>> -    /* supported mbus codes are the same in isp sink pad */
> >>> -    code->pad = RKISP1_ISP_PAD_SINK_VIDEO;
> >>> +    /* supported mbus codes are the same in isp video src pad */
> >>> +    code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> >>>       ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> >>>                      &dummy_cfg, code);
> >>
> >> Actually, is this really the true? AFAIR the ISP itself can only output
> >> either Bayer or YUV 4:2:2. The resizer can take YUV 4:2:2 at its input
> >> and output YUV 4:4:4, 4:2:2 and 4:2:0. Bayer capture is handled with
> >> resizer bypass mode. We haven't tested that, but if implemented, it
> >> should probably be done by exposing a link between the ISP entity and a
> >> video node directly, without the resizer involved.
> >>
> >> WDYT?
> >
> > We can also implement it that way. Only the mainpath needs
> > a direct link from the isp since selfpath does not support bayer formats.
> > It makes it easier on userspace for bayer formats since it does not have to
> > configure the resizer.
> > On the other hand if the format is YUV it has the option
> > to either use the the resizer or not.
> >
> > Thanks,
> > Dafna
>
> Anyway, this is a two line bug fix, so I think this patch can first be
> accepted and then if we choose to change the topology this can be done
> in a separate patchset.

Makes sense. Feel free to add my Reviewed-by.

Best regards,
Tomasz