On Tue, Mar 04, 2025 at 01:43:09AM +0200, Dmitry Baryshkov wrote:
> On Mon, Mar 03, 2025 at 10:45:19AM -0800, Jessica Zhang wrote:
> >
> >
> > On 2/27/2025 7:07 AM, Dmitry Baryshkov wrote:
> > > On Fri, Feb 14, 2025 at 04:14:26PM -0800, Jessica Zhang wrote:
> > > > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > >
> > > > Up to now the driver has been using encoder to allocate hardware
> > > > resources. Switch it to use CRTC id in preparation for the next step.
> > > >
> > > > Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
> > > > ---
> > > > Changes in v6:
> > > > - Drop duplicate cstate initialization code and unnecessary memset
> > > > Changes in v5:
> > > > - Reordered to prevent breaking CI and upon partial application
> > > >
> > > > Changes in v4 (due to rebase):
> > > > - moved *_get_assigned_resources() changes for DSPP and LM from
> > > > encoder *_virt_atomic_mode_set() to *_assign_crtc_resources()
> > > > ---
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 18 +--
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 +-
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 12 +-
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 189 ++++++++++++++--------------
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 7 +-
> > > > 5 files changed, 110 insertions(+), 126 deletions(-)
> > >
> > > This commit breaks several tests in CI:
> > > - sc7180-trogdor-kingoftown:
> > > - kms_cursor_crc@cursor-dpms
> > > - kms_pipe_crc_basic@disable-crc-after-crtc
> > > - sc7180-trogdor-lazor-limozeen
> > > - kms_cursor_crc@cursor-dpms
> > > - kms_pipe_crc_basic@disable-crc-after-crtc
> >
> > Hey Dmitry,
> >
> > Thanks for catching this. Looks like this was exposed due to a recent IGT
> > uprev that included dc2d7fb4f978 ("lib/igt_kms: move setting
> > DRM_CLIENT_CAP_WRITEBACK_CONNECTORS to kms_writeback").
> >
> > The issue itself is that when DPMS is toggled, it is possible for RM to
> > reserve new HW resources but skip the atomic_enable() due to the checks here
> > [1]. This means that the change in HW block reservation won't be propagated
> > to encoder if DPMS is set to off.
>
> Could you possibly clarify this. What is the state change that causes
> the issue (describe CRTC / connectors / encoders and active / enabled
> state). Why does the issue manifest only after switching to the CRTC id
> for resource allocation (the tests run successfully before this patch,
> i.e. with the resource allocation being moved to CRTC, but using the
> encoder ID for allocation).
>
> Note, the CRTC won't re-allocate resources if
> drm_atomic_crtc_needs_modeset() is not true. So I'm not sure how can we
> end up in a situation when the resources are reallocated _and_ we need
> to raise the mode_changed flag.
I'm going to apply the following fixup on top of your series (to the
previous patch), fixing the DPMS issue. It makes all SC7180 tests pass
in CI.
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 ++-
1 file changed, 2 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 ba5c60296e17..10653bd52885 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1373,7 +1373,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
- if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+ /* don't reallocate resources if only ACTIVE has beeen changed */
+ if (crtc_state->mode_changed || crtc_state->connectors_changed) {
rc = dpu_crtc_assign_resources(crtc, crtc_state);
if (rc < 0)
return rc;
--
2.39.5
>
> > I've posted a fix for this here [2].
> >
> > Thanks,
> >
> > Jessica Zhang
> >
> > [1] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/gpu/drm/drm_atomic_helper.c#L1502
> > [2] https://patchwork.freedesktop.org/series/145735/
> >
> > >
> > > Corresponding pipeline is available at [1]
> > >
> > > As I had to rebase your changes on top of msm-next, corresponding tree
> > > is available at [2]. It might be possible that the regression is
> > > introduced by my rebase.
> > >
> > > [1] https://gitlab.freedesktop.org/drm/msm/-/pipelines/1374165
> > >
> > > [2] https://gitlab.freedesktop.org/lumag/msm/-/commits/msm-next-lumag-cwb
> > >
> > > --
> > > With best wishes
> > > Dmitry
> >
>
> --
> With best wishes
> Dmitry
--
With best wishes
Dmitry
