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'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
