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

[Ville Syrjälä]

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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > >> ---
> > >>   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@xxxxxxxxx>
> > >> @@ -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...

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

-- 
Ville Syrjälä
Intel