| Message ID | 20240829-concurrent-wb-v1-7-502b16ae2ebb@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: Add Concurrent Writeback Support for DPU 10.x+ | expand |
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@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@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. > + 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 >
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@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@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). > >> + DPU_ERROR("crtc%d failed valid clone check for mask 0x%x\n", > > DPU_DEBUG, don't let users spam dmesg. Ack. > >> + 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. Ack Thanks, Jessica Zhang > >> + 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
On Wed, 4 Sept 2024 at 01:18, Jessica Zhang <quic_jesszhan@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@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@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. -- With best wishes Dmitry
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@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@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@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... Are there decent IGTs to make sure the kernel properly rejects illegal cloning configurations?
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@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)) + num_cwb_sessions++; + + /* + * Only support a single concurrent writeback session running + * at a time + */ + 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) { + DPU_ERROR("crtc%d failed valid clone check for mask 0x%x\n", + 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)) + 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,
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@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 36 +++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)