Re: [PATCH v9 27/28] media: iris: enable video driver probe of SM8250 SoC

[Dmitry Baryshkov]

On Mon, Feb 03, 2025 at 09:39:00AM +0100, Johan Hovold wrote:
> On Fri, Jan 10, 2025 at 08:01:21PM +0200, Dmitry Baryshkov wrote:
> > On 10 January 2025 19:30:30 EET, Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx> wrote:
> > >
> > >
> > >On 1/10/2025 7:58 PM, Johan Hovold wrote:
> > >> On Thu, Jan 09, 2025 at 11:18:29PM +0530, Vikash Garodia wrote:
> > >>> On 1/9/2025 8:41 PM, 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.)
> > >> 
> > >>> This was discussed during v4 [1] and implemented as per suggestion
> > >>>
> > >>> [1]
> > >>> https://lore.kernel.org/linux-media/eea14133-2152-37bb-e2ff-fcc7ed4c47f5@xxxxxxxxxxx/
> > >> 
> > >> First, the background and motivation for this still needs to go in the
> > >> commit message (and be mentioned in the cover letter).
> > >> 
> > >> Second, what you implemented here is not even equivalent to what was
> > >> done in the mdm drm driver since that module parameter is honoured by
> > >> both drivers so that at most one driver tries to bind to the platform
> > >> device.
> > >> 
> > >> With this patch as it stands, which driver ends up binding depends on
> > >> things like link order and what driver has been built a module, etc. (as
> > >> I pointed out below).
> > >> 
> > >>>> 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)?
> > >> 
> > >>> Its about the platform in migration i.e sm8250. Since new driver is not yet
> > >>> feature parity with old driver, choice is provided to client if it wants to use
> > >>> the new driver (default being old driver for sm8250)
> > >> 
> > >> This should be described in the commit message, along with details on
> > >> what the delta is so that the reasoning can be evaluated.
> > >> 
> > >> And I'm still not sure using a module parameter for this is the right
> > >> thing to do as it is generally something that should be avoided.
> > >> 
> > >I understand your concern of using module params.
> > >I will modify it to rely on Kconfig to select the driver (suggested by
> > >Hans) instead of module param.
> > 
> > Please don't. This makes it impossible to perform side-by-side
> > comparison.
> 
> Why? You can have two kernel builds and run the same tests. And you
> obviously cannot run iris and venus on the same hardware at once anyway.

At once not. But unbindng and rebinding another driver works perfectly.

> > Also as venus and iris drivers are not completely
> > equivalent wrt supported platforms, distributions will have to select
> > whether to disable support for older platforms or for new platforms:
> > Kconfig dependency will make it impossible to enable support for both
> > kinds.
> 
> You shouldn't have both enabled. The only reason for keeping support
> for the same hardware in both drivers is that the iris support is
> incomplete and considered experimental. No one should enable that except
> for development and evaluation purposes until the driver is up to par.
> And then you drop support from the old driver along with the config
> option.

That's the plan. This modparam is a temporal thing for transition
period. And yes, as a developers / platform enablers we want to be able
to have a quick turnaround between drivers.

Please stop forcing your decisions on other people. The Linux kernel and
its development process has always been about providing a possibility,
not a policy.

> 
> Johan

-- 
With best wishes
Dmitry