Re: [Intel-gfx] [RFC 2/7] drm: allow drm_wait_vblank_kernel() callback from workqueues

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Nov 19, 2014 at 05:47:10PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This is going to be needed by i915.ko, and I guess other drivers could
> use it too.

You may want to explain what we plan to use this for in i915 so that
other driver developers will more easily see where it might apply to
their own drivers.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c           | 46 ++++++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_debugfs.c | 45 ++++++++++++++++++++++++++++--------
>  include/drm/drmP.h                  | 11 ++++++++-
>  3 files changed, 86 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index f031f77..099aef1 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1108,6 +1108,22 @@ void drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_crtc_wait_one_vblank);
>  
> +static void drm_wait_vblank_callback(struct drm_device *dev,
> +				     struct drm_pending_vblank_event *e,
> +				     unsigned long seq, struct timeval *now,
> +				     bool premature)
> +{
> +	if (e->callback_from_work) {
> +		e->callback_args.dev = dev;
> +		e->callback_args.seq = seq;
> +		e->callback_args.now = now;
> +		e->callback_args.premature = premature;
> +		schedule_work(&e->callback_work);

As I noted on your alternative implementation, do we need to let the
driver choose the workqueue it wants to wait on?  We're always going to
use the system workqueue here, but some places in i915 already use a
private workqueue.


> +	} else {
> +		e->callback(dev, e, seq, now, premature);
> +	}
> +}
> +
...
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 39d0d87..bc114f0 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -669,7 +669,16 @@ struct drm_pending_vblank_event {
>  	struct drm_pending_event base;
>  	int pipe;
>  	struct drm_event_vblank event;
> +
>  	drm_vblank_callback_t callback;
> +	bool callback_from_work;
> +	struct work_struct callback_work;
> +	struct {
> +		struct drm_device *dev;
> +		unsigned long seq;
> +		struct timeval *now;

Do we need seq and now here?  We still have access to the
drm_pending_vblank_event in our callback, so can't we just use the
fields we already have in e->event?

Also, do we need dev for something specific?  Can't the driver just grab
that from its user_data structure?


Matt


> +		bool premature;
> +	} callback_args;
>  };
>  
>  struct drm_vblank_crtc {
> @@ -906,7 +915,7 @@ extern int drm_vblank_init(struct drm_device *dev, int num_crtcs);
>  extern int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  				 struct drm_file *filp);
>  extern int drm_wait_vblank_kernel(struct drm_crtc *crtc, int count,
> -				  bool absolute,
> +				  bool absolute, bool callback_from_work,
>  				  drm_vblank_callback_t callback,
>  				  unsigned long user_data);
>  extern u32 drm_vblank_count(struct drm_device *dev, int crtc);
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel