On Wed, Jan 15, 2025 at 10:49:28PM +0000, Bryan O'Donoghue wrote:
> On 10/01/2025 00:12, Dmitry Baryshkov wrote:
> > On Thu, Jan 09, 2025 at 04:11:04PM +0100, Johan Hovold wrote:
> > > On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote:
> > > > Initialize the platform data and enable video driver probe of SM8250
> > > > SoC. Add a kernel param to select between venus and iris drivers for
> > > > platforms supported by both drivers, for ex: SM8250.
> > >
> > > Why do you want to use a module parameter for this? What would be the
> > > default configuration? (Module parameters should generally be avoided.)
> > >
> > > Why not simply switch to the new driver (and make sure that the new
> > > driver is selected if the old one was enabled in the kernel config)?
> >
> > Because the new driver doesn't yet have feature parity with the venus
> > driver. So it was agreed that developers provide upgrade path to allow
> > users to gradually test and switch to the new driver. When the feature
> > parity is achieved, the plan is to switch default to point to the Iris
> > driver, then after a few releases start removing platforms from Venus.
> >
> > > > Tested-by: Stefan Schmidt <stefan.schmidt@xxxxxxxxxx> # x1e80100 (Dell
> > >
> > > Looks like something is missing from Stefan's Tested-by tag throughout
> > > the series ("Dell XPS13"?)
> > >
> > > > Reviewed-by: Stefan Schmidt <stefan.schmidt@xxxxxxxxxx>
> > > > Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> > > > +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);
> > > > +
> > > > +/* list all platforms supported by only iris driver */
> > > > +static const char *const iris_only_platforms[] = {
> > > > + "qcom,sm8550-iris",
> > > > + NULL,
> > > > +};
> > >
> > > Surely you don't want to have to add every new platform to two tables
> > > (i.e. the id table and again here).
> >
> > I'd agree here, this list should go. We should only list platforms under
> > the migration.
> >
> > >
> > > > +
> > > > +/* list all platforms supported by both venus and iris drivers */
> > > > +static const char *const venus_to_iris_migration[] = {
> > > > + "qcom,sm8250-venus",
> > > > + NULL,
> > > > +};
> > > > +
> > > > +static bool video_drv_should_bind(struct device *dev, bool is_iris_driver)
> >
> > The name should follow other names in the driver.
> > `video_drv_should_bind` doesn't have a common prefix.
> >
> > Also export it and use it from the venus driver if Iris is enabled.
> >
> > > > +{
> > > > + if (of_device_compatible_match(dev->of_node, iris_only_platforms))
> > > > + return is_iris_driver;
> > > > +
> > > > + /* If it is not in the migration list, use venus */
> > > > + if (!of_device_compatible_match(dev->of_node, venus_to_iris_migration))
> > > > + return !is_iris_driver;
> > > > +
> > > > + return prefer_venus ? !is_iris_driver : is_iris_driver;
> > > > +}
> > > > +
> > > > static int iris_probe(struct platform_device *pdev)
> > > > {
> > > > struct device *dev = &pdev->dev;
> > > > @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev)
> > > > u64 dma_mask;
> > > > int ret;
> > > > + if (!video_drv_should_bind(&pdev->dev, true))
> > > > + return -ENODEV;
> > >
> > > AFAICT nothing is preventing venus from binding even when 'prefer_venus'
> > > is false.
> > >
> > > > +
> > > > core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
> > > > if (!core)
> > > > return -ENOMEM;
> > > > @@ -324,6 +355,10 @@ static const struct of_device_id iris_dt_match[] = {
> > > > .compatible = "qcom,sm8550-iris",
> > > > .data = &sm8550_data,
> > > > },
> > > > + {
> > > > + .compatible = "qcom,sm8250-venus",
> > > > + .data = &sm8250_data,
> > > > + },
> > > > { },
> > > > };
> > > > MODULE_DEVICE_TABLE(of, iris_dt_match);
> > >
> > > Johan
> >
>
> One drawback to this approach is; if CONFIG_QCOM_VENUS=n and
> CONFIG_QCOM_IRIS=m you still need to-do
>
> zcat /proc/config.gz | grep -e VENUS -e IRIS
> CONFIG_VIDEO_QCOM_IRIS=m
> # CONFIG_VIDEO_QCOM_VENUS is not set
>
> rmmod iris
> modprobe iris prefer_venus=0
>
> which is awkward.
>
> Certainly if venus is off the parameter should default to false.
Not just the parameter. The whole function should become a stub if
either iris or venus is off.
> Perhaps I've missed the resolution of this discussion but how exactly are we
> fixing the "racy" nature of binding here ?
>
> Shouldn't there be a parallel venus patch which either has its own module
> parameter to quiesce binding in favour of iris ?
Venus should call video_drv_should_bind() too. Possibly it's worth
separating this function and a table to a helper kernel module, so that
the venus driver doesn't suddenly get a runtime dependency on iris if
both are enabled.
>
> i.e if
>
> CONFIG_QCOM_VENUS=m and CONFIG_QCOM_IRIS=m
>
> rmmod iris
> rmmod venus
>
> (sleep $((RANDOM % 3600)); (modprobe iris prefer_venus=0) &> /dev/null &
> disown) &
>
> (sleep $((RANDOM % 3600)); (modprobe venus) &> /dev/null & disown) &
>
> Will do what exactly ?
Nice question. I'd add iris.prefer_venus=0 to kernel commandline and let
it live there for a transition period. It should fix the possible race
condition here.
--
With best wishes
Dmitry
