[Intel-gfx] [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen wrote:
> On Tue,  2 Mar 2021 12:41:32 -0800
> Manasi Navare <manasi.d.navare at intel.com> 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.
> 
> Hi,
> 
> I think that makes sense to me. Stealing CRTCs that are not currently
> used by the userspace (display server) should be ok, as long as that
> is completely invisible to userspace: meaning that it does not cause
> userspace to unexpectedly e.g. receive or miss per-crtc atomic
> completion events.

Yes since we are only doing atomic_check_only() here, the stolen
crtc is completely invisible to the userspace and hence that is 
indicated by uapi.enable which is not true for this stolen
crtc. However if allow modeset flag set, then it will do a full
modeset and indicate the uapi.enable for this stolen crtc as well
since that cannot be used for other modeset requested by userspace.

> 
> Can that also be asserted somehow, or does this already do that?

Not clear what you want the assertion for? Could you elaborate

Manasi

> 
> 
> Thanks,
> pq
> 
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Simon Ser <contact at emersion.fr>
> > Cc: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > Cc: Daniel Stone <daniels at collabora.com>
> > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: dri-devel at lists.freedesktop.org
> > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 5b4547e0f775..d7acd6bbd97e 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1358,8 +1358,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
>