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

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

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@quicinc.com/
> 
> 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.
something like:
config VIDEO_QCOM_IRIS
        tristate "Qualcomm iris V4L2 decoder driver"
       ...
        depends on VIDEO_QCOM_VENUS=n || COMPILE_TEST

Thanks,
Dikshita
>>>>  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;
> 
> Johan