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

[Dmitry Baryshkov]

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.

> > Solution: If the user prefers iris driver and iris driver has not probed 
> > yet, and if venus tries to probe ahead of iris we keep -EDEFERing till 
> > iris probes and succeeds. Vice-versa when the preference is venus as well.
> 
> This sounds wrong too.
> 
> Look, first you guys need to explain *why* you want to have two drivers
> for the same hardware (not just to me, in the commit message and cover
> letter).
>
> That's something that really should never be the case and would need to
> be motivated properly.

I think it has been written several time (not sure about this commit):
to provide a way for a migration path _while_ people are working on Iris
features. Being able to A/B test Venus and Iris drivers and to report
possible regressions or lack of those on the common platforms supported
by those (sm8250 at this moment).

> Second, if the reasons for keeping both drivers are deemed justifiable,
> you need to come up with mechanism for only binding one of them.
> 
> I already told you that module parameters is not the way to go here (and
> the msm drm driver's abuse of module parameters is not a good precedent
> here).
> 
> If this is a transitional thing (which it must be), then just add a
> Kconfig symbol to determine which driver should probe. That's good
> enough for evaluating whatever needs to be evaluated, and doesn't
> depend on adding anti-patterns like module parameters (and helper
> modules for them).

No, Kconfig complicates A/B testing. What you usually want to do is to
boot a kernel, then go over a bunch of files testing that they work with
both drivers. Kconfig implies booting abother kernel, etc.

> 
> Keep it simple.
> 
> Johan

-- 
With best wishes
Dmitry