Hi,
On 07.07.2020 19:55, Tomasz Figa wrote:
> On Sat, Apr 25, 2020 at 07:26:42PM -0700, Jonathan Bakker wrote:
>> Not all devices use the CSIS device, some may use the FIMC directly in
>> which case the CSIS device isn't registered. This leads to a nullptr
>> exception when starting the stream as the CSIS device is always
>> referenced. Instead, if getting the CSIS device fails, try getting the
>> FIMC directly to check if we are using the subdev API
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@xxxxxxx>
>> ---
>> drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++-
>> 1 file changed, 16 insertions(+), 1 deletion(-)
> Thank you for the patch. Please see my comments inline.
>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>> index 9aaf3b8060d5..5c32abc7251b 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>> @@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on)
>> { IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP },
>> };
>> struct fimc_pipeline *p = to_fimc_pipeline(ep);
>> - struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity);
>> enum fimc_subdev_index sd_id;
>> int i, ret = 0;
>>
>> if (p->subdevs[IDX_SENSOR] == NULL) {
>> + struct fimc_md *fmd;
>> + struct v4l2_subdev *sd = p->subdevs[IDX_CSIS];
>> +
>> + if (!sd)
>> + sd = p->subdevs[IDX_FIMC];
>> +
>> + if (!sd) {
>> + /*
>> + * If neither CSIS nor FIMC was set up,
>> + * it's impossible to have any sensors
>> + */
>> + return -ENODEV;
>> + }
>> +
>> + fmd = entity_to_fimc_mdev(&sd->entity);
>> +
>
> Are you sure this is the correct thing to do here? In general, the media
> controller should be instantiated only if there are sensors in the system.
The code being changed is only about getting a pointer to the driver private
data structure 'struct fimc_md', for that we need to get hold of a media
entity that represents a subdev that is actually available in the pipeline.
In original code it is overlooked that there might camera pipelines without
the CSIS subdev.
> What do you mean by using "the FIMC directly"? Do you mean using it only as
> an m2m image processor or with a sensor, but without the CSIS, which would
> be the case for parallel I/F sensors?
I think it is about a use case where the sensor is connected directly to the
FIMC capture interface (parallel), without the MIPI-CSI2 receiverin between.
> Could you point me to the place where CSIS is always dereferenced? A quick
> look through the code only revealed that everywhere it seems to be guarded
> by a NULL check.
It's in this patch, above, the very first line that the patch removes.
> Another thought from looking at the implementation of
> __fimc_pipeline_s_stream() is that it probably shouldn't call s_stream on
> all the subdevices included in seq[], but only on those that are actually
> included as a part of the pipeline. It would be quite a waste of power to
> enable unnecessary hardware.
As we talked on IRC, only subdev in current active media pipeline will be
powered on/off. The p->subdevs[] array is sparsely populated and there is
a test for NULL in __subdev_set_power(). Perhaps the test should be moved
to the caller instead (fimc_pipeline_s_power()), so we don't ignore any
ENXIO errors from the s_power v4l2_subdev_call. But is a material for
a separate patch.
--
Regards,
Sylwester
