On Tue, Jan 18, 2022 at 06:34:20PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 22, 2021 at 12:51:12PM -0700, Navare, Manasi wrote:
> >
> > Hi Ville,
> >
> > Could you take a look at this, this addresses teh review comments from prev version
>
> I don't think I ever got an answer to my question as to whether this
> was tested with all the interesting scenarios:
> 1) just with the master crtc added by userspace into the commit
> 2) just with the slave crtc added by userspace into the commit
> 3) both crtcs added by userspace into the commit
>
> I guess 1) has been tested since that happens all the time, but the other
> two scanarios would likely need to be done with a synthetic test to make
> sure we're actually hitting them.
>
> I think it *should* work, but I'd like to have real proof of that.
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Thank you for your review here Ville.
I have triggered a separate email thread to understand all the above testing scenarios and get them tested with bigjoiner IGT.
Manasi
>
> >
> > Manasi
> >
> > On Mon, Oct 04, 2021 at 04:59:13AM -0700, Manasi Navare wrote:
> > > In case of a modeset where a mode gets split across mutiple CRTCs
> > > in the driver specific implementation (bigjoiner in i915) we wrongly count
> > > the affected CRTCs based on the drm_crtc_mask and indicate the stolen CRTC as
> > > an affected CRTC in atomic_check_only().
> > > This triggers a warning since affected CRTCs doent match requested CRTC.
> > >
> > > To fix this in such bigjoiner configurations, we should only
> > > increment affected crtcs if that CRTC is enabled in UAPI not
> > > if it is just used internally in the driver to split the mode.
> > >
> > > v3: Add the same uapi crtc_state->enable check in requested
> > > crtc calc (Ville)
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Simon Ser <contact@xxxxxxxxxxx>
> > > Cc: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 12 ++++++++----
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index ff1416cd609a..a1e4c7905ebb 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1310,8 +1310,10 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> > >
> > > DRM_DEBUG_ATOMIC("checking %p\n", state);
> > >
> > > - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > - requested_crtc |= drm_crtc_mask(crtc);
> > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > + if (new_crtc_state->enable)
> > > + requested_crtc |= drm_crtc_mask(crtc);
> > > + }
> > >
> > > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> > > ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
> > > @@ -1360,8 +1362,10 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> > > }
> > > }
> > >
> > > - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > - affected_crtc |= drm_crtc_mask(crtc);
> > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > + if (new_crtc_state->enable)
> > > + affected_crtc |= drm_crtc_mask(crtc);
> > > + }
> > >
> > > /*
> > > * For commits that allow modesets drivers can add other CRTCs to the
> > > --
> > > 2.19.1
> > >
>
> --
> Ville Syrjälä
> Intel
