Message-ID: <4ca3d1a6-4895-489a-87be-f883a001bc7c@quicinc.com>
Date: Sat, 7 Jun 2025 10:31:03 +0800
From: Renjiang Han <quic_renjiang@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        Bryan O'Donoghue
	<bryan.odonoghue@...aro.org>,
        Krzysztof Kozlowski <krzk@...nel.org>
CC: Vikash Garodia <quic_vgarodia@...cinc.com>,
        Dikshita Agarwal
	<quic_dikshita@...cinc.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio
	<konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, <linux-media@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        "Nicolas
 Dufresne" <nicolas.dufresne@...labora.com>
Subject: Re: [PATCH v8 0/3] media: venus: enable venus on qcs615


On 6/7/2025 10:13 AM, Dmitry Baryshkov wrote:
> On 06/06/2025 15:53, Bryan O'Donoghue wrote:
>> On 06/06/2025 14:32, Renjiang Han wrote:
>>>
>>> On 6/6/2025 8:56 PM, Krzysztof Kozlowski wrote:
>>>> On 06/06/2025 14:51, Renjiang Han wrote:
>>>>> On 6/6/2025 8:44 PM, Krzysztof Kozlowski wrote:
>>>>>> On 06/06/2025 14:37, Renjiang Han wrote:
>>>>>>> On 6/5/2025 8:34 PM, Bryan O'Donoghue wrote:
>>>>>>>> On 31/05/2025 01:05, Renjiang Han wrote:
>>>>>>>>>>> Note:
>>>>>>>>>>> This series consist of DT patches and a venus driver patch. 
>>>>>>>>>>> The patch
>>>>>>>>>>> 1/3, which is venus driver patch, can be picked 
>>>>>>>>>>> independently without
>>>>>>>>>>> having any functional dependency. But patch 2/3 & patch 3/3, 
>>>>>>>>>>> which are
>>>>>>>>>>> DT patches, still depend on [1].
>>>>>>>>>> I'd say 2/3 and 3/3 still depend on 1/3, otherwise we can get 
>>>>>>>>>> video
>>>>>>>>>> core
>>>>>>>>>> on QCS615 over(?)clocked.
>>>>>>>>> Agree, so we need to make sure that the driver patch is not 
>>>>>>>>> picked
>>>>>>>>> after the DT patch.
>>>>>>>> This statement is confusing.
>>>>>>>>
>>>>>>>> 1/3 states that there will be a fallback if there is no OPP table
>>>>>>>> present.
>>>>>>>>
>>>>>>>> Giving the code a glance, I believe that is so, freq_table 
>>>>>>>> should be
>>>>>>>> used if there is no OPP specified in the DT.
>>>>>>>>
>>>>>>>> I think we are having a hard time here understanding what you 
>>>>>>>> are saying.
>>>>>>>>
>>>>>>>> My understanding:
>>>>>>>>
>>>>>>>> - venus modification is standalone 1/3
>>>>>>>>     Qcs615 will fallback if no OPP is present
>>>>>>>>
>>>>>>>> - dt modification 2/3 3/3 is therefore also independent of driver
>>>>>>>>
>>>>>>>> ---
>>>>>>>> bod
>>>>>>> yes, let me re-spin this with driver patch alone. Once that gets 
>>>>>>> in,
>>>>>>> will bring in the DT patches.
>>>>>> Did you read my feedback? There is no "once that gets in". DTS is an
>>>>>> independent hardware description and your patchset claiming there is
>>>>>> dependency is just broken.
>>>>>>
>>>>>> I am repeating this since few emails, so shall I NAK it that you 
>>>>>> will
>>>>>> address the main issue you have?
>>>>>>
>>>>>> Best regards,
>>>>>> Krzysztof
>>>>> Hi Krzysztof
>>>>>
>>>>> SC7180 and QCS615 use the same video core. Only difference lies in 
>>>>> the
>>>>> freq_table for the video. Freq_table is generally determined at 
>>>>> SOC level.
>>>>> The Venus driver does not currently handle freq_table 
>>>>> compatibility well
>>>>> across platforms. This patch enables the driver to use the 
>>>>> OPP-table from
>>>>> the DT, addressing the frequency compatibility issue.
>>>> This does not resolve the main problem at all. If SW cannot use the
>>>> fallback alone, your fallback has no meaning and is not only confusing
>>>> but actually incorrect. And based on previous statements like
>>>> "overclocking" it is not only incorrect, but even harmful.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>> The fallback is only triggered when there is no OPP table in the DT.
>>> Since the QCS615 DT will include an OPP table, the fallback logic will
>>> not be used.
>>>
>>> Also, if the freq from the freq_table and the OPP table are the same,
>>> would it be acceptable to drop the freq_table from the driver?
>
> No, it's not acceptable, because then you'll break support for old 
> DTs, which is a no-go.
>
>>
>> If you drop the freq_table, you will need to apply OPPs for the 
>> sc7180 to DTS first before venus or you'll break sc7180.
>>
>> I think TBH you should add a freq_tbl for QCS615 and make it so the 
>> order of patch application doesn't matter wrt adding OPP support.
>
> That would require adding board data for QCS615, which definitely 
> doesn't look like a good solution. It is _exactly_ the same as SC7180. 
> I'm against duplicating it just for the sake of having freq_tbl.
>
>>
>> - Add QCS freq_tbl
>> - Add OPP support
>>
>> Then do whatever in DTS, nothing can break in this case.
>>
>> As we've established the fallback isn't a fallback because it falls 
>> back to wrong data, so lets fix that.
>
> Why isn't it a fallback? With the driver changes in place, the 
> fallback is totally correct.
Agree.
Also, currently, both the freq_table and the opp-table are present
in the driver and DT code, and their frequency values are identical.
This driver patch is simply switching the frequency source from freq_table
to opp-table.
Therefore, I think this patch is an enhancement to the driver's
functionality and can be submitted independently, as it is not part of any
specific project.

-- 
Best Regards,
Renjiang

Powered by blists - more mailing lists