[PATCH] libcamera: rkisp1: Eliminate hard-coded resizer limits

[Umang Jain]

Hi Jacopo

On 26/09/24 2:51 pm, Jacopo Mondi wrote:
> Hi Umang
>
> On Tue, Sep 24, 2024 at 12:43:41PM GMT, Umang Jain wrote:
>> The resizer limits are dynamically queried in populateFormats() and
> Is this the resizer ?
>
> minResolution_ and maxResolution_ are populated from the list of
> formats from video_ and if I recall correctly RkISP1 has an explicit
> resizer entity, right ?

Ah yes, you are correct. I will need to update the commit message.

The minResolution_ and maxResolution_ are populated from 
(main|self)_path video nodes.

I would go with:

```
     libcamera: rkisp1: Eliminate hard-coded resizer limits

     The minResolution_ and maxResolution_ limits are dynamically queried
     in populateFormats() from the RkISP1Path video node. Therefore,
     initializing these limits with the resizer limits in the constructor is
     unnecessary.

     This change allows us to remove the hard-coded max/min resolution 
limits
     of the resizer from RkISP1Path, simplifying its constructor further.

```
>
>> assigned to minResolution_ and maxResolution_. Therefore, initializing
>> these values in the constructor is unnecessary.
>>
>> This change allows us to remove the hard-coded max/min resolution limits
>> from RkISP1Path, simplifying its constructor further.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 21 ++++++-------------
>>   src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 +--
>>   2 files changed, 7 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> index c49017d1..8015c58c 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> @@ -54,11 +54,8 @@ const std::map<PixelFormat, uint32_t> formatToMediaBus = {
>>
>>   } /* namespace */
>>
>> -RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>> -		       const Size &minResolution, const Size &maxResolution)
>> -	: name_(name), running_(false), formats_(formats),
>> -	  minResolution_(minResolution), maxResolution_(maxResolution),
>> -	  link_(nullptr)
>> +RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats)
>> +	: name_(name), running_(false), formats_(formats), link_(nullptr)
>>   {
>>   }
>>
>> @@ -435,12 +432,10 @@ void RkISP1Path::stop()
>>   }
>>
>>   /*
>> - * \todo Remove the hardcoded resolutions and formats once all users will have
>> - * migrated to a recent enough kernel.
>> + * \todo Remove the hardcoded formats once all users will have migrated to a
>> + * recent enough kernel.
>>    */
>>   namespace {
>> -constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
>> -constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
>>   constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
>>   	formats::YUYV,
>>   	formats::NV16,
>> @@ -462,8 +457,6 @@ constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
>>   	formats::SRGGB12,
>>   };
>>
>> -constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
>> -constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
>>   constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{
>>   	formats::YUYV,
>>   	formats::NV16,
>> @@ -477,14 +470,12 @@ constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{
>>   } /* namespace */
>>
>>   RkISP1MainPath::RkISP1MainPath()
>> -	: RkISP1Path("main", RKISP1_RSZ_MP_FORMATS,
>> -		     RKISP1_RSZ_MP_SRC_MIN, RKISP1_RSZ_MP_SRC_MAX)
>> +	: RkISP1Path("main", RKISP1_RSZ_MP_FORMATS)
>>   {
>>   }
>>
>>   RkISP1SelfPath::RkISP1SelfPath()
>> -	: RkISP1Path("self", RKISP1_RSZ_SP_FORMATS,
>> -		     RKISP1_RSZ_SP_SRC_MIN, RKISP1_RSZ_SP_SRC_MAX)
>> +	: RkISP1Path("self", RKISP1_RSZ_SP_FORMATS)
>>   {
>>   }
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>> index 08edefec..d33471e4 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>> @@ -32,8 +32,7 @@ struct V4L2SubdeviceFormat;
>>   class RkISP1Path
>>   {
>>   public:
>> -	RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>> -		   const Size &minResolution, const Size &maxResolution);
>> +	RkISP1Path(const char *name, const Span<const PixelFormat> &formats);
> However I think this is safe. minResolution_ and maxResolution_ are
> populated at ::init() time and the class is not usable before that
> function has been called.
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
>    j
>
>>   	bool init(MediaDevice *media);
>>
>> --
>> 2.45.2
>>