Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS
 device present
To:     Tomasz Figa <tfiga@chromium.org>,
        Jonathan Bakker <xc-racer2@live.ca>
Cc:     kyungmin.park@samsung.com, mchehab@kernel.org, kgene@kernel.org,
        krzk@kernel.org, linux-media@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org
From:   Sylwester Nawrocki <s.nawrocki@samsung.com>
Message-ID: <854c0c60-b239-6626-2423-47058cb58feb@samsung.com>
Date:   Wed, 8 Jul 2020 17:11:46 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
        Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200707175517.GC2621465@chromium.org>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20200426022650.10355-1-xc-racer2@live.ca>
        <BN6PR04MB0660EE7304C2BB2E603A8824A3AE0@BN6PR04MB0660.namprd04.prod.outlook.com>
        <CGME20200707175524eucas1p29d92419590ac09cc48de3336b834e69b@eucas1p2.samsung.com>
        <20200707175517.GC2621465@chromium.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@live.ca>
>> ---
>>  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