Re: [RFC PATCH v10 1/2] media: iris: introduce helper module to select video driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/02/2025 16:16, Dmitry Baryshkov wrote:
> On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote:
>> On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote:
>>> On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote:
>>>> On 28/01/2025 09:04, Dikshita Agarwal wrote:
>>
>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>>>>> index 954cc7c0cc97..276461ade811 100644
>>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>>>>> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev)
>>>>>   	u64 dma_mask;
>>>>>   	int ret;
>>>>>   
>>>>> +	if (!video_drv_should_bind(&pdev->dev, true))
>>>>> +		return -ENODEV;
>>>>
>>>> Wouldn't it mark the probe as failed and cause dmesg regressions?
>>
>> No, this is perfectly fine. Probe can return -ENODEV and driver core
>> will continue with any further matches.
>>
>>>>> +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS)
>>>>> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver)
>>>>> +{
>>>>> +	/* If just a single driver is enabled, use it no matter what */
>>>>> +	return true;
>>>>> +}
>>>>> +
>>>>> +#else
>>>>> +static bool prefer_venus = true;
>>>>> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred");
>>>>> +module_param(prefer_venus, bool, 0444);
>>>>
>>>>
>>>> The choice of driver is by module blacklisting, not by failing probes.
>>>>
>>>> I don't understand why this patchset is needed and neither commit msg
>>>> nor above longer code comment explain me that. Just blacklist the module.
>>
>>> Summarizing the discussion with myself, Krzysztof and Dmitry:
>>>
>>> 1) module blacklisting solution will not be ideal if users want to have 
>>> both venus and iris or either of them built-in
>>
>> Module blacklisting is not the way to go, you shouldn't have two drivers
>> racing to bind to the same device ever.
>>
>>> 2) with current approach, one of the probes (either venus or iris) will 
>>> certainly fail as video_drv_should_bind() will fail for one of them. 
>>> This can be considered as a regression and should not happen.
>>
>> How can that be a regression? One driver must fail to probe (see above).
> 
> I also don't think that it's a regression. I think Krzysztof was
> concerned about the 'failed to bind' messages in dmesg.

I never used word "regression" alone. I said "dmesg regression", which
means you have error in logs or any system facility which provides you
self-information about device probe history. I don't remember if -ENODEV
leads to any printks, so maybe I am wrong here, but regardless normal
and expected operation of a driver should never result in -ERRNO, except
deferred probe of course.

Best regards,
Krzysztof