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

[Daniel Vetter]

On Wed, Jul 05, 2017 at 03:10:12PM -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.
> 
> Signed-off-by: Keith Packard <keithp@xxxxxxxxxx>

A few nits below, but looks good otherwise.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c         |  7 ++++---
>  drivers/gpu/drm/drm_plane.c          |  2 +-
>  drivers/gpu/drm/drm_vblank.c         | 27 ++++++++++++++++-----------
>  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, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c0f336d23f9c..f569d7f03f3c 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_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 *.

>  {
>  	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(dev, 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;
> +		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 f55f997c0b8f..9ae170857ef6 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -807,13 +807,18 @@ static void send_vblank_event(struct drm_device *dev,
>  		struct drm_pending_vblank_event *e,
>  		u64 seq, struct timespec *now)
>  {
> -	e->event.sequence = seq;
> -	e->event.tv_sec = now->tv_sec;
> -	e->event.tv_usec = now->tv_nsec / 1000;
> -
> -	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:
> +		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.

> +	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>  	drm_send_event_locked(dev, &e->base);
>  }
>  
> @@ -865,7 +870,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);
> @@ -897,7 +901,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);
> @@ -1342,6 +1345,7 @@ 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);

This'll oops on ums drivers since kms isn't set up. And we can call this
from ums drivers in the old vblank ioctl. My long-term goal is to
completely separate these two worlds, and exclusive deal with drm_crtc *
in the kms side (and only compute the pipe index when needed). But we're
not there yet. I also want to split it into two files (drm_crtc_vblank.c
and drm_legacy_vblank.c) and make sure we consistently use
drm_crtc_vblank_ as the new-world prefix.

Probably the simplest option is to extend this to take all three (dev,
pipe, crtc) as arguments, and then pass NULL for the old vblank ioctl, and
the right pointer for the new stuff. Or you stuff a DRIVER_MODESET check
at the right spot somewhere.

Or maybe I shouldn't have told you this and seized this opportunity to
break all the old drivers :-)

>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  	struct drm_pending_vblank_event *e;
>  	struct timespec now;
> @@ -1357,8 +1361,9 @@ 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 = crtc ? crtc->base.id : 0;

>  
>  	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 68e99177fff3..3ef7ddc5db5f 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,
> +			  struct timespec *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.11.0
> 

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