On Wed, Jun 10, 2020 at 9:00 PM Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > >>> Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx> > >>> --- > >>> 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
