[PATCH 07/21] drm/msm/dpu: Check CRTC encoders are valid clones

[Jessica Zhang]

On 9/4/2024 12:23 PM, Ville Syrjälä wrote:
> On Wed, Sep 04, 2024 at 09:41:23PM +0300, Dmitry Baryshkov wrote:
>> On Wed, 4 Sept 2024 at 01:18, Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
>>>
>>>
>>>
>>> On 8/30/2024 10:00 AM, Dmitry Baryshkov wrote:
>>>> On Thu, Aug 29, 2024 at 01:48:28PM GMT, Jessica Zhang wrote:
>>>>> Check that each encoder in the CRTC state's encoder_mask is marked as a
>>>>> possible clone for all other encoders in the encoder_mask and that only
>>>>> one CRTC is in clone mode at a time
>>>>>
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 36 +++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 35 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> index 5ec1b5a38922..bebae365c036 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> @@ -1,6 +1,6 @@
>>>>>    // SPDX-License-Identifier: GPL-2.0-only
>>>>>    /*
>>>>> - * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>     * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>>>>>     * Copyright (C) 2013 Red Hat
>>>>>     * Author: Rob Clark <robdclark at gmail.com>
>>>>> @@ -1204,6 +1204,36 @@ static struct msm_display_topology dpu_crtc_get_topology(
>>>>>       return topology;
>>>>>    }
>>>>>
>>>>> +static bool dpu_crtc_has_valid_clones(struct drm_crtc *crtc,
>>>>> +            struct drm_crtc_state *crtc_state)
>>>>> +{
>>>>> +    struct drm_encoder *drm_enc;
>>>>> +    struct drm_crtc *temp_crtc;
>>>>> +    int num_cwb_sessions = 0;
>>>>> +
>>>>> +    drm_for_each_crtc(temp_crtc, crtc->dev)
>>>>> +            if (drm_crtc_in_clone_mode(temp_crtc->state))
>>>>
>>>> No, get the state from drm_atomic_state. temp_crtc->state might be
>>>> irrelevant.
>>>
>>> Hi Dmitry,
>>>
>>> Ack.
>>>
>>>>
>>>>> +                    num_cwb_sessions++;
>>>>
>>>> Even simpler:
>>>> if (temp_crtc != crtc && drm_crtc_in_clone_mode(...))
>>>>        return false;
>>>
>>> Ack.
>>>
>>>>
>>>>> +
>>>>> +    /*
>>>>> +     * Only support a single concurrent writeback session running
>>>>> +     * at a time
>>>>
>>>> If it is not a hardware limitation, please add:
>>>> FIXME: support more than one session
>>>
>>> This is a hardware limitation.
>>>
>>>>
>>>>> +     */
>>>>> +    if (num_cwb_sessions > 1)
>>>>> +            return false;
>>>>> +
>>>>> +    drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc_state->encoder_mask) {
>>>>> +            if ((crtc_state->encoder_mask & drm_enc->possible_clones) !=
>>>>> +                            crtc_state->encoder_mask) {
>>>>
>>>> Align to opening bracket, please. Granted that other drivers don't
>>>> perform this check, is it really necessary? Doesn't
>>>> validate_encoder_possible_clones() ensure the same, but during the
>>>> encoder registration?
>>>
>>> The difference here is that validate_encoder_possible_clones() is only
>>> called when the drm device is initially registered.
>>>
>>> The check here is to make sure that the encoders userspace is proposing
>>> to be cloned are actually possible clones of each other. This might not
>>> be necessary for drivers where all encoders are all possible clones of
>>> each other. But for MSM (and CWB), real-time display encoders can only
>>> be clones of writeback (and vice versa).
>>
>> I had the feeling that encoder_mask should already take care of that,
>> but it seems I was wrong.
>> Please extract this piece as a generic helper. I think it should be
>> called from the generic atomic_check() codepath.
> 
> Yeah, if we are semi-assured that drivers aren't screwing up those
> bitmasks anymore we could shove the cloning checks into
> drm_atomic_helper_check_modeset(). It already checks possible_crtcs.
> We could then throw out the equavalent code from i915 as well...

Hi Ville and Dmitry,

Sure, I can move this part of the check to *_atomic_check_modeset() -- 
I'll add it here [1] since it's after the encoder mask is updated.

[1] 
https://elixir.bootlin.com/linux/v6.10.7/source/drivers/gpu/drm/drm_atomic_helper.c#L731

> 
> Are there decent IGTs to make sure the kernel properly rejects
> illegal cloning configurations?

There's a few invalid clone subtests in kms_setmode [2]

Thanks,

Jessica Zhang

[2] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_setmode.c#L63

> 
> -- 
> Ville Syrjälä
> Intel