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.
--
With best wishes
Dmitry
