[PATCH v2 1/2] libcamera rkisp1: Reduce scope of sensor max resolution variable

[Umang Jain]

Hi Kieran

On 04/10/24 3:15 am, Kieran Bingham wrote:
> Quoting Umang Jain (2024-10-03 14:57:39)
>> Reduce the scope of sensor's max resolution variable in
>> RkISP1Path::validate(), as it is only required to constraint the
>> maximum and minimum resolution bounds on the non-RAW code path.
>>
>> No functional changes intended in this patch.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>> ---
>>   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> index cb6117b9..e7f1f12a 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> @@ -254,7 +254,6 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>>                                                   StreamConfiguration *cfg)
>>   {
>>          const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>> -       Size resolution = filterSensorResolution(sensor);
>>   
>>          const StreamConfiguration reqCfg = *cfg;
>>          CameraConfiguration::Status status = CameraConfiguration::Valid;
>> @@ -322,6 +321,8 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> But in here - I do wonder if this is another location that should be
> restricted to the limits of the ISP.
>
> Bringing in surrounding context:
>
>          if (isRaw) {
>                  /*
>                   * Use the sensor output size closest to the requested stream
>                   * size.
>                   */
>                  uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
>                  V4L2SubdeviceFormat sensorFormat =
>                          sensor->getFormat({ mbusCode }, cfg->size);
>
>
> I wonder if that section in (isRaw) also needs to be clamped to the
> filteredSensorResolution, or should only pick the closest fit from the
> list of modes available in the list that got filtered...
>
> So I think moving this line has only highlighted to me that we've missed
> a bit.

We have and now there is a fix:

[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on 
raw path

We missed the clamping because it didn't existed in the first place 
when  I wrote

https://git.libcamera.org/libcamera/libcamera.git/commit/?id=761545407c7666063f4c98d92a17af2a2c790b08

But I should have noticed :-/

>
> No objection to still merging this anyway - but I think something might
> need to be adapted in here.
>
>
>
>>                  minResolution = sensorFormat.size;
>>                  maxResolution = sensorFormat.size;
>>          } else {
>> +               Size resolution = filterSensorResolution(sensor);
>> +
>>                  /*
>>                   * Adjust the size based on the sensor resolution and absolute
>>                   * limits of the ISP.
>> -- 
>> 2.45.2
>>