[PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline

[Umang Jain]

On 12/09/24 4:18 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-09-12 11:29:40)
>> Hi Laurent
>>
>> On 12/09/24 3:59 am, Laurent Pinchart wrote:
>>> On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote:
>>>> Quoting Umang Jain (2024-09-09 17:37:19)
>>>>> It is possible that the maximum sensor size (returned by
>>>>> CameraSensor::resolution()) is not supported by the pipeline. In such
>>>>> cases, a filter function is required to filter out resolutions of the
>>>>> camera sensor, which cannot be supported by the pipeline.
>>>>>
>>>>> Introduce the filter function filterSensorResolution() in RkISP1Path
>>>>> class and use it for validate() and generateConfiguration(). The
>>>>> maximum sensor resolution is picked from that filtered set of
>>>>> resolutions instead of CameraSensor::resolution().
>>>>>
>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>> ---
>>>>>    src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++-
>>>>>    src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 ++
>>>>>    2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>>> index 9053af18..b3b27aa5 100644
>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()
>>>>>           }
>>>>>    }
>>>>>    
>>>>> +/**
>>>>> + * \brief Filter the sensor resolutions that can be supported
>>>>> + * \param[in] sensor The camera sensor
>>>>> + *
>>>>> + * This function retrieves all the sizes supported by the sensor and
>>>>> + * filters all the resolutions that can be supported on the pipeline.
>>>>> + * It is possible that the sensor's maximum output resolution is higher
>>>>> + * than the ISP maximum input. In that case, this function filters out all
>>>>> + * the resolution incapable of being supported and returns the maximum
>>>>> + * sensor resolution that can be supported by the pipeline.
>>>>> + *
>>>>> + * \return Maximum sensor size supported on the pipeline
>>>>> + */
>>>>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)
>>>>> +{
>>>>> +       if (!sensorSizes_.empty())
>>>>> +               return sensorSizes_.back();
>>> There are systems based on rkisp1 where multiple sensors are connected
>>> to the same ISP. Obviously only one of them can be used at a time, but
>>> applications can switch between them. This will be broken by caching
>>> sensorSizes_.
>> Indeed, something that I missed while developing on i.MX8MP (two cameras
>> but dual ISP as well)
>>
>> I'll convert it to a std::map in next revision.
> Rather than a map - can we just put this filter/or the sensorSizes_ into
> the camera data?

hmm, might require passing CameraData into RkISP1Path::validate(...)

and

RkISP1Path:: generateConfiguration(const CameraSensor *sensor,....)

Possibly replacing the CameraSensor function argument in both.

>
>
>
>>>> I like that we keep the full list of supported sensorSizes_. My first
>>>> thought was that this list should be generated when the path is created,
>>>> but this also seems fine to me.
>>>>
>>>> I think there's no need to complicate / split this until there's
>>>> actually a user of the full sensorSizes list ?
>>>>
>>>> I'm a little surprised that's not not used actually. Do we report the
>>>> raw modes we support somewhere? Should that iterate the 'sensorSizes_'
>>>> as the supported sizes ?
>> I will test how raw sizes are reported ? sensorSizes_ is a good
>> candidate to update the raw size list as well.
> I'm intrigued on what it's currently reporting...
>
> --
> Kieran
>
>
>>>> Perhaps that's an improvement/fix on top though.
>>>>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>>
>>>>> +
>>>>> +       std::vector<Size> sensorSizes;
>>>>> +       const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>>>>> +       for (const auto iter : mbusCodes) {
>>>>> +               std::vector<Size> sizes = sensor->sizes(iter);
>>>>> +               for (Size sz : sizes)
>>>>> +                       sensorSizes.push_back(sz);
>>>>> +       }
>>>>> +
>>>>> +       std::sort(sensorSizes.begin(), sensorSizes.end());
>>>>> +
>>>>> +       /* Remove duplicates. */
>>>>> +       auto last = std::unique(sensorSizes.begin(), sensorSizes.end());
>>>>> +       sensorSizes.erase(last, sensorSizes.end());
>>>>> +
>>>>> +       /* Discard any sizes that the pipeline is unable to support. */
>>>>> +       for (auto sz : sensorSizes) {
>>>>> +               if (sz.width > maxResolution_.width ||
>>>>> +                   sz.height > maxResolution_.height)
>>>>> +                       continue;
>>>>> +
>>>>> +               sensorSizes_.push_back(sz);
>>>>> +       }
>>>>> +
>>>>> +       return sensorSizes_.back();
>>>>> +}
>>>>> +
>>>>>    StreamConfiguration
>>>>>    RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>>>>>                                     StreamRole role)
>>>>>    {
>>>>>           const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>>>>> -       const Size &resolution = sensor->resolution();
>>>>> +       Size resolution = filterSensorResolution(sensor);
>>>>>    
>>>>>           /* Min and max resolutions to populate the available stream formats. */
>>>>>           Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
>>>>> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>>>>>                                                    StreamConfiguration *cfg)
>>>>>    {
>>>>>           const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>>>>> -       const Size &resolution = sensor->resolution();
>>>>> +       Size resolution = filterSensorResolution(sensor);
>>>>>    
>>>>>           const StreamConfiguration reqCfg = *cfg;
>>>>>           CameraConfiguration::Status status = CameraConfiguration::Valid;
>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>>>>> index 13ba4b62..03ff9543 100644
>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
>>>>> @@ -63,6 +63,7 @@ public:
>>>>>    
>>>>>    private:
>>>>>           void populateFormats();
>>>>> +       Size filterSensorResolution(const CameraSensor *sensor);
>>>>>    
>>>>>           static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>>>>>    
>>>>> @@ -77,6 +78,8 @@ private:
>>>>>           std::unique_ptr<V4L2Subdevice> resizer_;
>>>>>           std::unique_ptr<V4L2VideoDevice> video_;
>>>>>           MediaLink *link_;
>>>>> +
>>>>> +       std::vector<Size> sensorSizes_;
>>>>>    };
>>>>>    
>>>>>    class RkISP1MainPath : public RkISP1Path