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

From: Daniel Vetter <daniel@ffwll.ch>
To: Keith Packard <keithp@keithp.com>
Cc: linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types [v2]
Date: Wed, 2 Aug 2017 11:05:45 +0200	[thread overview]
Message-ID: <20170802090545.3r5kukes7xgrazsc@phenom.ffwll.local> (raw)
In-Reply-To: <20170801050306.24423-3-keithp@keithp.com>

On Mon, Jul 31, 2017 at 10:03:05PM -0700, Keith Packard wrote:
> Place drm_event_vblank in a new union that includes that and a bare
> drm_event structure. This will allow new members of that union to be
> added in the future without changing code related to the existing vbl
> event type.
> 
> Assignments to the crtc_id field are now done when the event is
> allocated, rather than when delievered. This way, delivery doesn't
> need to have the crtc ID available.
> 
> v2:
>  * Remove 'dev' argument from create_vblank_event
> 
> 	It wasn't being used anyways, and if we need it in the future,
> 	we can always get it from crtc->dev.
> 
>  * Check for MODESETTING before looking for crtc in queue_vblank_event
> 
> 	UMS drivers will oops if we try to get a crtc, so make sure
> 	we're modesetting before we try to find a crtc_id to fill into
> 	the event.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/drm_atomic.c         |  7 ++++---
>  drivers/gpu/drm/drm_plane.c          |  2 +-
>  drivers/gpu/drm/drm_vblank.c         | 30 ++++++++++++++++++------------
>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  4 ++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  4 ++--
>  include/drm/drm_vblank.h             |  8 +++++++-
>  6 files changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c0f336d23f9c..272b83ea9369 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor)
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> -		struct drm_device *dev, uint64_t user_data)
> +		struct drm_crtc *crtc, uint64_t user_data)
>  {
>  	struct drm_pending_vblank_event *e = NULL;
>  
> @@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event(
>  
>  	e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
>  	e->event.base.length = sizeof(e->event);
> -	e->event.user_data = user_data;
> +	e->event.vbl.crtc_id = crtc->base.id;
> +	e->event.vbl.user_data = user_data;
>  
>  	return e;
>  }
> @@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
>  		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
>  			struct drm_pending_vblank_event *e;
>  
> -			e = create_vblank_event(dev, arg->user_data);
> +			e = create_vblank_event(crtc, arg->user_data);
>  			if (!e)
>  				return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 5dc8c4350602..fe9f31285bc2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		}
>  		e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
>  		e->event.base.length = sizeof(e->event);
> -		e->event.user_data = page_flip->user_data;

You missed assigning crtc_id here. With that fixes:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Might also be good to check igt coverage for the various corner-cases
here.

> +		e->event.vbl.user_data = page_flip->user_data;
>  		ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
>  		if (ret) {
>  			kfree(e);
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 346601ad698d..7e7119a5ada3 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -804,14 +804,16 @@ static void send_vblank_event(struct drm_device *dev,
>  {
>  	struct timeval tv;
>  
> -	tv = ktime_to_timeval(now);
> -	e->event.sequence = seq;
> -	e->event.tv_sec = tv.tv_sec;
> -	e->event.tv_usec = tv.tv_usec;
> -
> -	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
> -					 e->event.sequence);
> -
> +	switch (e->event.base.type) {
> +	case DRM_EVENT_VBLANK:
> +	case DRM_EVENT_FLIP_COMPLETE:
> +		tv = ktime_to_timeval(now);
> +		e->event.vbl.sequence = seq;
> +		e->event.vbl.tv_sec = tv.tv_sec;
> +		e->event.vbl.tv_usec = tv.tv_usec;
> +		break;
> +	}
> +	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>  	drm_send_event_locked(dev, &e->base);
>  }
>  
> @@ -863,7 +865,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  
>  	e->pipe = pipe;
>  	e->sequence = drm_vblank_count(dev, pipe);
> -	e->event.crtc_id = crtc->base.id;
>  	list_add_tail(&e->base.link, &dev->vblank_event_list);
>  }
>  EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
> @@ -894,7 +895,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  		now = get_drm_timestamp();
>  	}
>  	e->pipe = pipe;
> -	e->event.crtc_id = crtc->base.id;
>  	send_vblank_event(dev, e, seq, now);
>  }
>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
> @@ -1354,8 +1354,14 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  
>  	e->pipe = pipe;
>  	e->event.base.type = DRM_EVENT_VBLANK;
> -	e->event.base.length = sizeof(e->event);
> -	e->event.user_data = vblwait->request.signal;
> +	e->event.base.length = sizeof(e->event.vbl);
> +	e->event.vbl.user_data = vblwait->request.signal;
> +	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);
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 8d7dc9def7c2..c13b97338310 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc,
>  
>  		ret = vmw_event_fence_action_queue(file_priv, fence,
>  						   &event->base,
> -						   &event->event.tv_sec,
> -						   &event->event.tv_usec,
> +						   &event->event.vbl.tv_sec,
> +						   &event->event.vbl.tv_usec,
>  						   true);
>  	}
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index bad31bdf09b6..4e329588ce9c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc,
>  
>  		ret = vmw_event_fence_action_queue(file_priv, fence,
>  						   &event->base,
> -						   &event->event.tv_sec,
> -						   &event->event.tv_usec,
> +						   &event->event.vbl.tv_sec,
> +						   &event->event.vbl.tv_usec,
>  						   true);
>  		vmw_fence_obj_unreference(&fence);
>  	} else {
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index e809ab244919..3013c55aec1d 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -54,7 +54,10 @@ struct drm_pending_vblank_event {
>  	/**
>  	 * @event: Actual event which will be sent to userspace.
>  	 */
> -	struct drm_event_vblank event;
> +	union {
> +		struct drm_event base;
> +		struct drm_event_vblank vbl;
> +	} event;
>  };
>  
>  /**
> @@ -163,6 +166,9 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  			       struct drm_pending_vblank_event *e);
>  void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  			      struct drm_pending_vblank_event *e);
> +void drm_vblank_set_event(struct drm_pending_vblank_event *e,
> +			  u64 *seq,
> +			  ktime_t *now);
>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>  bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>  int drm_crtc_vblank_get(struct drm_crtc *crtc);
> -- 
> 2.13.3
> 

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