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

[Stanimir Varbanov <svarbanov@suse.de>]

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@quicinc.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@quicinc.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.

~Stan