Re: [PATCH v3] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

["Navare, Manasi" <manasi.d.navare@intel.com>]

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@linux.intel.com>

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@linux.intel.com>
> > > Cc: Simon Ser <contact@emersion.fr>
> > > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> > > Cc: Daniel Stone <daniels@collabora.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  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