diff mbox series

[1/2] media: rkisp1: Increase ISP input resolution limit

Message ID 20210325192700.21393-2-sebastian.fricke@posteo.net (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers
Series Allow higher sink pad resolutions for the RkISP1 |

Commit Message

Sebastian Fricke March 25, 2021, 7:27 p.m. UTC 
  The incentive for this patch was to overcome the inability of the
ISP device to work with resolutions greater than 4032x3024.
Increase the limit to 4416x3312 to allow higher input resolutions.
Use the old resolution to crop the input resolution down to the maximum
size for the ISP to process frames.

Signed-off-by: Sebastian Fricke <sebastian.fricke@posteo.net>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart March 26, 2021, 4:14 a.m. UTC | #1 
Hi Sebastian,

Thank you for the patch.

On Thu, Mar 25, 2021 at 08:27:01PM +0100, Sebastian Fricke wrote:
> The incentive for this patch was to overcome the inability of the
> ISP device to work with resolutions greater than 4032x3024.
> Increase the limit to 4416x3312 to allow higher input resolutions.
> Use the old resolution to crop the input resolution down to the maximum
> size for the ISP to process frames.
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke@posteo.net>
> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 038c303a8aed..553effda4d5e 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -30,10 +30,12 @@
>  #define RKISP1_ISP_SD_SINK BIT(1)
>  
>  /* min and max values for the widths and heights of the entities */
> -#define RKISP1_ISP_MAX_WIDTH		4032
> -#define RKISP1_ISP_MAX_HEIGHT		3024
> -#define RKISP1_ISP_MIN_WIDTH		32
> -#define RKISP1_ISP_MIN_HEIGHT		32
> +#define RKISP1_ISP_MAX_WIDTH			4416
> +#define RKISP1_ISP_MAX_HEIGHT			3312
> +#define RKISP1_ISP_MAX_WIDTH_PROCESSING		4032
> +#define RKISP1_ISP_MAX_HEIGHT_PROCESSING	3024

Maybe s/_PROCESSING/_CROP/, or a name more descriptive ?

I would also squash this patch with 2/2, as otherwise you'll introduce a
bisection breakage (applying 1/2 only will leave the driver in an
incorrect state).

> +#define RKISP1_ISP_MIN_WIDTH			32
> +#define RKISP1_ISP_MIN_HEIGHT			32
>  
>  #define RKISP1_RSZ_MP_SRC_MAX_WIDTH		4416
>  #define RKISP1_RSZ_MP_SRC_MAX_HEIGHT		3312
Sebastian Fricke March 26, 2021, 5:55 a.m. UTC | #2 
Hey Laurent,

On 26.03.2021 06:14, Laurent Pinchart wrote:
>Hi Sebastian,
>
>Thank you for the patch.

Thank you for your review.

>
>On Thu, Mar 25, 2021 at 08:27:01PM +0100, Sebastian Fricke wrote:
>> The incentive for this patch was to overcome the inability of the
>> ISP device to work with resolutions greater than 4032x3024.
>> Increase the limit to 4416x3312 to allow higher input resolutions.
>> Use the old resolution to crop the input resolution down to the maximum
>> size for the ISP to process frames.
>>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@posteo.net>
>> ---
>>  drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>> index 038c303a8aed..553effda4d5e 100644
>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>> @@ -30,10 +30,12 @@
>>  #define RKISP1_ISP_SD_SINK BIT(1)
>>
>>  /* min and max values for the widths and heights of the entities */
>> -#define RKISP1_ISP_MAX_WIDTH		4032
>> -#define RKISP1_ISP_MAX_HEIGHT		3024
>> -#define RKISP1_ISP_MIN_WIDTH		32
>> -#define RKISP1_ISP_MIN_HEIGHT		32
>> +#define RKISP1_ISP_MAX_WIDTH			4416
>> +#define RKISP1_ISP_MAX_HEIGHT			3312
>> +#define RKISP1_ISP_MAX_WIDTH_PROCESSING		4032
>> +#define RKISP1_ISP_MAX_HEIGHT_PROCESSING	3024
>
>Maybe s/_PROCESSING/_CROP/, or a name more descriptive ?

My thought behind this name was the difference between the maximum size
that the ISP accepts as an input and the maximum size that it can
actually process. So more descriptive in that case would be:
`RKISP1_ISP_MAX_HEIGHT_FOR_PROCESSING` but this is way too long.
I guess `RKISP1_ISP_MAX_HEIGHT_CROP` is a good short name, that also
makes sense at the place where it is used. I guess I should add a
comment though, to explain why the crop has that limitation.

>
>I would also squash this patch with 2/2, as otherwise you'll introduce a
>bisection breakage (applying 1/2 only will leave the driver in an
>incorrect state).

Oh yes, I will do that.

>
>> +#define RKISP1_ISP_MIN_WIDTH			32
>> +#define RKISP1_ISP_MIN_HEIGHT			32
>>
>>  #define RKISP1_RSZ_MP_SRC_MAX_WIDTH		4416
>>  #define RKISP1_RSZ_MP_SRC_MAX_HEIGHT		3312
>
>-- 
>Regards,
>
>Laurent Pinchart

Greetings,

Sebastian Fricke
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 038c303a8aed..553effda4d5e 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -30,10 +30,12 @@ 
 #define RKISP1_ISP_SD_SINK BIT(1)
 
 /* min and max values for the widths and heights of the entities */
-#define RKISP1_ISP_MAX_WIDTH		4032
-#define RKISP1_ISP_MAX_HEIGHT		3024
-#define RKISP1_ISP_MIN_WIDTH		32
-#define RKISP1_ISP_MIN_HEIGHT		32
+#define RKISP1_ISP_MAX_WIDTH			4416
+#define RKISP1_ISP_MAX_HEIGHT			3312
+#define RKISP1_ISP_MAX_WIDTH_PROCESSING		4032
+#define RKISP1_ISP_MAX_HEIGHT_PROCESSING	3024
+#define RKISP1_ISP_MIN_WIDTH			32
+#define RKISP1_ISP_MIN_HEIGHT			32
 
 #define RKISP1_RSZ_MP_SRC_MAX_WIDTH		4416
 #define RKISP1_RSZ_MP_SRC_MAX_HEIGHT		3312