Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types

From: Daniel Vetter <daniel@ffwll.ch>
To: Keith Packard <keithp@keithp.com>
Cc: Daniel Vetter <daniel@ffwll.ch>,
	linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types
Date: Fri, 7 Jul 2017 14:05:42 +0200	[thread overview]
Message-ID: <20170707120542.a7neukhlldpzj3j6@phenom.ffwll.local> (raw)
In-Reply-To: <864lupd1cv.fsf@keithp.com>

On Thu, Jul 06, 2017 at 08:36:00AM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > A few nits below, but looks good otherwise.
> 
> Thanks.
> 
> >>  static struct drm_pending_vblank_event *create_vblank_event(
> >> -		struct drm_device *dev, uint64_t user_data)
> >> +		struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data)
> >
> > Nit: Please also drop the dev argument, we have crtc->dev easily
> > available. That fits better into my long-term goal of getting rid of the
> > (dev, pipe) pairs everywhere in the vblank code and fully switching over
> > to drm_crtc *.
> 
> As 'dev' isn't used anyways, this seems like a fine plan.
> 
> >> +	switch (e->event.base.type) {
> >> +	case DRM_EVENT_VBLANK:
> >> +	case DRM_EVENT_FLIP_COMPLETE:
> >> +		if (seq)
> >> +			e->event.vbl.sequence = (u32) seq;
> >> +		if (now) {
> >> +			e->event.vbl.tv_sec = now->tv_sec;
> >> +			e->event.vbl.tv_usec = now->tv_nsec / 1000;
> >> +		}
> >> +		break;
> >> +	}
> >
> > Not sure why this change? Also prep for the new, presumably extended
> > events? Seems at least slightly inconsistent with other paths, where we
> > still unconditionally fill it in.
> 
> Yes, this prepares for the new events to make that patch smaller. The
> places where the data are still unconditionally assigned should know
> that the event in the struct is either a VBLANK or FLIP_COMPLETE.

Yeah, I realized that after reading the next patch carefully.

> >> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> >
> > This'll oops on ums drivers since kms isn't set up.
> 
> How about this fix?
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 857b7cf011e1..e39b2bd074e4 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1355,7 +1355,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  				  union drm_wait_vblank *vblwait,
>  				  struct drm_file *file_priv)
>  {
> -	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e;
>  	struct timespec now;
> @@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	e->event.base.type = DRM_EVENT_VBLANK;
>  	e->event.base.length = sizeof(e->event.vbl);
>  	e->event.vbl.user_data = vblwait->request.signal;
> -	e->event.vbl.crtc_id = crtc ? crtc->base.id : 0;
> +	e->event.vbl.crtc_id = 0;
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +		struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> +		if (crtc)
> +			e->event.vbl.crtc_id = crtc->base.id;
> +	}
>  
>  	spin_lock_irqsave(&dev->event_lock, flags);

lgtm.

> > Or maybe I shouldn't have told you this and seized this opportunity to
> > break all the old drivers :-)
> 
> You now know my evil plan :-)

:-)

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch