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.
> + num_cwb_sessions++;
Even simpler:
if (temp_crtc != crtc && drm_crtc_in_clone_mode(...))
return false;
> +
> + /*
> + * 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
> + */
> + 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?
> + DPU_ERROR("crtc%d failed valid clone check for mask 0x%x\n",
DPU_DEBUG, don't let users spam dmesg.
> + crtc->base.id, crtc_state->encoder_mask);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> static int dpu_crtc_assign_resources(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state)
> {
> struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_CRTC];
> @@ -1287,6 +1317,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
> return rc;
> }
>
> + if (drm_crtc_in_clone_mode(crtc_state) &&
> + !dpu_crtc_has_valid_clones(crtc, crtc_state))
Align to opening bracket.
> + return -EINVAL;
> +
> if (!crtc_state->enable || !drm_atomic_crtc_effectively_active(crtc_state)) {
> DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n",
> crtc->base.id, crtc_state->enable,
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
