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 for Android: free password hash cracker in your pocket

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

Message-ID: <0d69e79f-e14f-4636-8360-8b5c2a6c2735@xs4all.nl>
Date: Thu, 9 Jan 2025 11:37:09 +0100
From: Hans Verkuil <hverkuil@...all.nl>
To: Stanimir Varbanov <svarbanov@...e.de>,
 Dikshita Agarwal <quic_dikshita@...cinc.com>,
 Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: Nicolas Dufresne <nicolas@...fresne.ca>,
 Sebastian Fricke <sebastian.fricke@...labora.com>,
 Vikash Garodia <quic_vgarodia@...cinc.com>,
 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>,
 Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 Neil Armstrong <neil.armstrong@...aro.org>,
 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
Subject: Re: [PATCH v9 27/28] media: iris: enable video driver probe of SM8250
 SoC

On 09/01/2025 10:49, Stanimir Varbanov wrote:
> Hi,
> 
> On 1/9/25 10:43 AM, Dikshita Agarwal wrote:
>>
>>
>> On 1/8/2025 8:22 PM, Mauro Carvalho Chehab wrote:
>>> Em Wed, 8 Jan 2025 16:42:03 +0530
>>> Dikshita Agarwal <quic_dikshita@...cinc.com> escreveu:
>>>
>>>> On 1/8/2025 4:13 PM, Hans Verkuil wrote:
>>>>> On 1/8/25 11:21, Dikshita Agarwal wrote:  
>>>>>>
>>>>>>
>>>>>> On 1/8/2025 2:25 PM, Hans Verkuil wrote:  
>>>>>>> On 08/01/2025 09:51, Dikshita Agarwal wrote:  
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/8/2025 1:17 PM, Hans Verkuil wrote:  
>>>>>>>>> On 08/01/2025 08:43, Dikshita Agarwal wrote:  
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 1/7/2025 7:27 PM, Nicolas Dufresne wrote:  
>>>>>>>>>>> Le lundi 23 décembre 2024 à 16:21 +0530, Dikshita Agarwal a écrit :  
>>>>>>>>>>>>
>>>>>>>>>>>> On 12/23/2024 4:00 PM, Mauro Carvalho Chehab wrote:  
>>>>>>>>>>>>> Em Thu, 12 Dec 2024 17:21:49 +0530
>>>>>>>>>>>>> Dikshita Agarwal <quic_dikshita@...cinc.com> escreveu:
>>>>>>>>>>>>>  
>>>>>>>>>>>>>> +	.dma_mask = GENMASK(31, 29) - 1,  
>>>>>>>>>>>>>
>>>>>>>>>>>>> Setting a mask to GENMASK() - 1 sounds weird. Is it really what you want?
>>>>>>>>>>>>> I so, why?
>>>>>>>>>>>>>  
>>>>>>>>>>>> Hi Mauro,
>>>>>>>>>>>>
>>>>>>>>>>>> the value of this dma mask should be 0xe0000000 -1.
>>>>>>>>>>>>
>>>>>>>>>>>> The background for the same is, 0xe0000000 onward memory space is allocated
>>>>>>>>>>>> for IO register space so we are restricting the driver buffer allocations
>>>>>>>>>>>> to 0xe0000000 - 1.
>>>>>>>>>>>>
>>>>>>>>>>>> Based on the comments received in the past, we are using GENMASK to
>>>>>>>>>>>> generate 0xe0000000.
>>>>>>>>>>>>
>>>>>>>>>>>> Does this answer your query or I missed something?  
>>>>>>>>>>>
>>>>>>>>>>> I'm not sure it will do what you want. (0xe0000000 -1) matches ~BIT(29). Perhaps
>>>>>>>>>>> you wanted to use ~0xe0000000. 
>>>>>>>>>>>  
>>>>>>>>>> value of dma mask is coming as expected with GENMASK(31, 29) - 1
>>>>>>>>>>
>>>>>>>>>> qcom-iris aa00000.video-codec: dma_mask DFFFFFFF (0xe0000000 -1)  
>>>>>>>>>
>>>>>>>>> Isn't this just the equivalent of GENMASK(28, 0)? Can't you use that?  
>>>>>>>
>>>>>>> Too early in the morning, this suggestion was clearly wrong.
>>>>>>>  
>>>>>>>>>
>>>>>>>>> It's much easier to understand than GENMASK()-1.  
>>>>>>>>
>>>>>>>> Sure, I can use either ~GENMASK(29, 29) or ~BIT(29),  
>>>>>>>
>>>>>>> ~BIT(29).
>>>>>>>
>>>>>>> It's really weird to just disable a single bit, so I think some comments
>>>>>>> explaining why this mask is needed would be good (if there aren't comments
>>>>>>> already).
>>>>>>>  
>>>>>> I tested this some more, and seems ~BIT(29) doesn't work, as its still
>>>>>> conflicting with the register space.  
>>>>>
>>>>> Odd, perhaps a 64 vs 32 bit issue?
>>>>>   
>>>>>> Correct value would be GENMASK(31,30) + GENMASK(28,0) to set the exact bits
>>>>>> to get the desired value i.e 0xe0000000 -1  
>>>>> Honestly, in this case I would prefer to just go with the actual hex value
>>>>> 0xdfffffff together with an explanatory comment.
>>>>>   
>>>> We moved to GENMASK way to address comment on previous version, but sure
>>>> can directly use 0xdfffffff with a comment.
>>>
>>> If I understood it right, bits 0-31 can be used, but the hardware has some
>>> issue using bit 29 at the mask. Could you please comment why it can't be
>>> used?
>>>
>> That would not be a correct statement, We don't have issue with using BIT
>> 29 with mask but upper limit of DMA address available to use is 0xdfffffff.
> 
> I'd keep the original representation of the DMA address mask. This is
> not an register to represent it via GENMASK. It is used by the driver to
> set the hardware limitation on the upper bound of DMA address range.
> 
> .dma_mask = 0xe0000000 - 1
> 
> because we set it through dma_set_mask_and_coherent() which expects a
> mask we subtract 1.

Yes, I agree. I see the venus driver does the same thing, so let's follow
that example for consistency.

Regards,

	Hans

> 
> ~Stan
> 
>