Re: [PATCH] drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

[Sean Paul]

On Mon, Jun 19, 2017 at 11:35:28AM -0400, Harry Wentland wrote:
> On 2017-06-09 05:30 PM, Andrey Grodzovsky wrote:
> > Problem:
> > While running IGT kms_atomic_transition test suite i encountered
> > a hang in drmHandleEvent immidietly follwoing an atomic_commit.
> 
> s/immidietly/immediately/g
> s/follwoing/following/g
> 
> > After dumping the atomic state I relized that in this case there was
> > not even one CRTC attached to the state and only disabled
> > planes. This probably due to a commit which hadn't changed any property
> > which would require attaching crtc state. This means drmHandleEvent
> > will never wake up from read since without CRTC in atomic state
> > the event fd will not be singnaled.
> 
> s/singnaled/signaled/g
> 
> > This point to a bug in IGT but also DRM should gracefully
> > fail  such scenario so no hang on user side will happen.
> > 
> 
> Can we create an IGT fix for this to make sure this won't happen?
> 
> > Fix:
> > Explicitly fail by failing atomic_commit early in
> > drm_mode_atomic_commit where such problem can be identified.
> > 
> 
> The change seems reasonable to me but I would like to see some input
> from someone who's more familiar with the usermode side of things.

I wonder if there's ever a case where it might be desirable to generate an event
from a commit without a crtc. I don't know if anyone has explicitly said that an
event can only be generated from state with crtc.

I usually don't mind letting userspace shoot itself in the foot, so keep that in
mind :)

Sean


> 
> > Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky@xxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index a567310..32eae1c 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1933,7 +1933,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
> >  {
> >  	struct drm_crtc *crtc;
> >  	struct drm_crtc_state *crtc_state;
> > -	int i, ret;
> > +	int i, c = 0, ret;
> >  
> >  	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> >  		return 0;
> > @@ -1994,8 +1994,17 @@ static int prepare_crtc_signaling(struct drm_device *dev,
> >  
> >  			crtc_state->event->base.fence = fence;
> >  		}
> > +
> > +		c++;
> 
> Not sure if intentional, but I like it.
> 
> >  	}
> >  
> > +	/*
> > +	 * Having this flag means user mode pends on event which will never
> > +	 * reach due to lack of at least one CRTC for signaling
> > +	 */
> > +	if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> > +		return -EINVAL;
> > +
> >  	return 0;
> >  }
> >  
> > @@ -2179,6 +2188,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  		drm_mode_object_unreference(obj);
> >  	}
> >  
> > +
> > +
> 
> Remove these extraneous newlines.
> 
> Harry
> 
> >  	ret = prepare_crtc_signaling(dev, state, arg, file_priv, &fence_state,
> >  				     &num_fences);
> >  	if (ret)
> > 

-- 
Sean Paul, Software Engineer, Google / Chromium OS