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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <24334fb8-4d83-eb06-aee3-dfe1f8e4937b@quicinc.com>
Date: Fri, 10 Jan 2025 23:00:30 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Johan Hovold <johan@...nel.org>,
        Vikash Garodia
	<quic_vgarodia@...cinc.com>
CC: Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Mauro Carvalho Chehab
	<mchehab@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Philipp Zabel
	<p.zabel@...gutronix.de>,
        Hans Verkuil <hverkuil@...all.nl>,
        Sebastian Fricke
	<sebastian.fricke@...labora.com>,
        Bryan O'Donoghue
	<bryan.odonoghue@...aro.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Nicolas Dufresne
	<nicolas@...fresne.ca>,
        Uwe Kleine-König
	<u.kleine-koenig@...libre.com>,
        Jianhua Lu <lujianhua000@...il.com>,
        "Stefan
 Schmidt" <stefan.schmidt@...aro.org>,
        <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Bjorn Andersson <andersson@...nel.org>
Subject: Re: [PATCH v9 27/28] media: iris: enable video driver probe of SM8250
 SoC



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