Re: [RFC 3/7] drm: introduce struct drm_vblank_wait_item

From: Matt Roper <matthew.d.roper@intel.com>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org,
	Paulo Zanoni <paulo.r.zanoni@intel.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [RFC 3/7] drm: introduce struct drm_vblank_wait_item
Date: Thu, 4 Dec 2014 18:27:48 -0800	[thread overview]
Message-ID: <20141205022748.GB962@intel.com> (raw)
In-Reply-To: <1416426435-2237-5-git-send-email-przanoni@gmail.com>

On Wed, Nov 19, 2014 at 05:47:11PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> It's supposed to contain all the information that is required for both
> kernel and user space vblank wait items, but not hold any information
> required by just one of them.
> 
> For now, we just moved the struct members from one place to another,
> but the long term goal is that most of the drm.ko code will only
> handle "struct drm_vblank_wait_item", not knowing anything else. This
> will allow the callers to wrap this struct in their own private
> structures. This will happen in the next patches.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'd clarify the commit message here to say that it's *eventually* going
to contain all of the general data and be subclassed by callers.  On
first read, I was confused here since you still have to use
e->event.user_data until patch #7.

Personally, I'd also just squash patch #4 into this one.

...
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index bc114f0..b8bc55a 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -665,10 +665,8 @@ typedef void (*drm_vblank_callback_t)(struct drm_device *dev,
>  				      unsigned long seq, struct timeval *now,
>  				      bool premature);
>  
> -struct drm_pending_vblank_event {
> -	struct drm_pending_event base;
> +struct drm_vblank_wait_item {
>  	int pipe;
> -	struct drm_event_vblank event;

I know it's nitpicking, but the term 'item' seems a bit vague/ambiguous
to me.  Maybe something like drm_vblank_task or drm_vblank_job would be
slightly more descriptive?  In the same vein, I'd suggest something like
"drm_schedule_vblank_job()" in place of drm_wait_vblank_kernel() and
"drm_trigger_vblank_job()" in place of drm_wait_vblank_callback().  Of
course this is all pretty much personal opinion, so feel free to ignore
this suggestion if you disagree.  :-)

Matt

>  
>  	drm_vblank_callback_t callback;
>  	bool callback_from_work;
> @@ -681,6 +679,12 @@ struct drm_pending_vblank_event {
>  	} callback_args;
>  };
>  
> +struct drm_pending_vblank_event {
> +	struct drm_pending_event base;
> +	struct drm_event_vblank event;
> +	struct drm_vblank_wait_item item;
> +};
> +
>  struct drm_vblank_crtc {
>  	struct drm_device *dev;		/* pointer to the drm_device */
>  	wait_queue_head_t queue;	/**< VBLANK wait queue */
> -- 
> 2.1.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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