diff mbox series

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

Message ID 20200609152825.24772-2-dafna.hirschfeld@collabora.com (mailing list archive)
State New, archived
Headers show
Series media: staging: rkisp1: bugs fixes and vars renames | expand

Commit Message

Dafna Hirschfeld June 9, 2020, 3:28 p.m. UTC 
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(-)

Comments

Tomasz Figa June 10, 2020, 5:15 p.m. UTC | #1 
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?

Best regards,
Tomasz
Dafna Hirschfeld June 10, 2020, 6:36 p.m. UTC | #2 
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

> 
> Best regards,
> Tomasz
>
Dafna Hirschfeld June 10, 2020, 7 p.m. UTC | #3 
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.

Thanks,
Dafna



> 
>>
>> Best regards,
>> Tomasz
>>
Tomasz Figa June 10, 2020, 7:28 p.m. UTC | #4 
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
diff mbox series

Patch

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);