Re: [PATCH v2] drm/msm/dpu: Filter modes based on adjusted mode clock

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 29/10/2025 14:49, Neil Armstrong wrote:
> On 10/29/25 13:30, Dmitry Baryshkov wrote:
>> On Wed, Oct 29, 2025 at 10:40:25AM +0100, Neil Armstrong wrote:
>>> On 10/28/25 20:52, Dmitry Baryshkov wrote:
>>>> On Tue, Oct 28, 2025 at 09:42:57AM +0100, neil.armstrong@linaro.org 
>>>> wrote:
>>>>> On 5/7/25 03:38, Jessica Zhang wrote:
>>>>>> Filter out modes that have a clock rate greater than the max core 
>>>>>> clock
>>>>>> rate when adjusted for the perf clock factor
>>>>>>
>>>>>> This is especially important for chipsets such as QCS615 that have 
>>>>>> lower
>>>>>> limits for the MDP max core clock.
>>>>>>
>>>>>> Since the core CRTC clock is at least the mode clock (adjusted for 
>>>>>> the
>>>>>> perf clock factor) [1], the modes supported by the driver should 
>>>>>> be less
>>>>>> than the max core clock rate.
>>>>>>
>>>>>> [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/gpu/ 
>>>>>> drm/msm/disp/dpu1/dpu_core_perf.c#L83
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> Signed-off-by: Jessica Zhang <jessica.zhang@oss.qualcomm.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> - *crtc_clock -> *mode_clock (Dmitry)
>>>>>> - Changed adjusted_mode_clk check to use multiplication (Dmitry)
>>>>>> - Switch from quic_* email to OSS email
>>>>>> - Link to v1: https://lore.kernel.org/lkml/20241212-filter-mode- 
>>>>>> clock-v1-1-f4441988d6aa@quicinc.com/
>>>>>> ---
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 35 +++++++++++ 
>>>>>> +++++++---------
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +++
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      | 12 +++++++++
>>>>>>     3 files changed, 39 insertions(+), 11 deletions(-)
>>>>>>
>>>>>
>>>>> This test doesn't take in account if the mode is for a bonded DSI 
>>>>> mode, which
>>>>> is the same mode on 2 interfaces doubled, but it's valid since we 
>>>>> could literally
>>>>> set both modes separately. In bonded DSI this mode_clk must be 
>>>>> again divided bv 2
>>>>> in addition to the fix:
>>>>> https://lore.kernel.org/linux-arm-msm/20250923-modeclk-fix- 
>>>>> v2-1-01fcd0b2465a@oss.qualcomm.com/
>>>>
>>>>   From the docs:
>>>>
>>>>            * Since this function is both called from the check phase 
>>>> of an atomic
>>>>            * commit, and the mode validation in the probe paths it 
>>>> is not allowed
>>>>            * to look at anything else but the passed-in mode, and 
>>>> validate it
>>>>            * against configuration-invariant hardware constraints. 
>>>> Any further
>>>>            * limits which depend upon the configuration can only be 
>>>> checked in
>>>>            * @mode_fixup or @atomic_check.
>>>>
>>>> Additionally, I don't think it is correct to divide mode_clk by two. In
>>>> the end, the DPU processes the mode in a single pass, so the perf
>>>> constrains applies as is, without additional dividers.
>>>
>>> Sorry but this is not correct, the current check means nothing. If you
>>> enable 2 separate DSI outputs or enable then in bonded mode, the DPU
>>> processes it the same so the bonded doubled mode should be valid.
>>>
>>> The difference between separate or bonded DSI DPU-wise is only the
>>> synchronisation of vsyncs between interfaces.
>>
>> I think there is some sort of confusion. It might be on my side. Please
>> correct me if I'm wrong.
>>
>> Each CRTC requires certain MDP clock rate to function: to process pixel
>> data, for scanout, etc. This is captured in dpu_core_perf.c. The patch
>> in question verifies that the mode can actually be set, that MDP can
>> function at the required clock rate. Otherwise we end up in a situation
>> when the driver lists a particular mode, but then the atomic_check
>> rejects that mode.
> 
> A CRTC will be associated to 1 or multiple LMs, the LM is the actual block
> you want to check for frequency. Speaking of CRTC means nothing for the 
> DPU.
> 
> We should basically run a lightweight version of dpu_rm_reserve() in 
> mode_valid,
> and check against all the assigned blocks to see if we can handle the mode.
> 
> But is it worth it ? What did the original patch solve exactly ?
> 
> Do we have formal proof about which max clock frequency a complete HW 
> setup is able to support ?
> 
>>
>> With that in mind, there is a difference between independent and bonded
>> DSI panels: bonded panels use single CRTC, while independent panels use
>> two different CRTCs. As such (again, please correct me if I'm wrong),
>> we need 2x MDP clock for a single CRTC.
> 
> Any mode can use 1 or multiple LMs, in independent or bonded DSI. As I
> said the bonded DSI with a 2x mode will lead to __exactly__ the same setup
> as 2 independed DSI displays. And in bonded mode, you'll always have 2 LMs.
> 
>>
>>> So this check against the max frequency means nothing and should be
>>> removed, but we should solely rely on the bandwidth calculation instead.
>>
>> We need both. If you have a particular usecase which fails, lets discuss
>> it:
>>
>> - 2 DSI panels, resolution WxH, N Hz, the mode uses l LMs, m DSC units
>>    and foo bar baz to output.
>>
>> - The dpu_crtc_mode_valid() calculates the clock ABC, which is more than
>>    the max value of DEF
>>
>> - The actual modesetting results in a clock GHI, which is less than DEF
> 
> I don't understand what you need,

I have been asking for exact W, H, N, l, m, etc. numbers.

> in the current form the mode_valid will
> accept the 2 DSI displays as independent, while using them as bonded will
> use the exact same HW setup (resolution WxH, N Hz, the mode uses l LMs, 
> m DSC units)
> but the mode_valid with rejects it.

Which clock rate is being returned by _dpu_core_perf_calc_clk() while 
setting up two independent outputs and when setting up a single bonded 
output?
Which clock rate is being used by dpu_crtc_mode_valid() to reject the mode?

> 
> Neil
> 
>>
>>>
>>> Neil
>>>
>>>>
>>>>
>>>>> I'm trying to find a correct way to handle that, I have tried that:
>>>>> ===========================><========================================
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/ 
>>>>> gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> index 48c3aef1cfc2..6aa5db1996e3 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>
>>>
>>
> 


-- 
With best wishes
Dmitry