Re: [Intel-gfx] [PATCH 10/24] drm: Remove drm_pending_event->pid

[Sean Paul <seanpaul@chromium.org>]

On Wed, Mar 08, 2017 at 03:12:43PM +0100, Daniel Vetter wrote:
> We might as well dump the drm_file pointer, that's about as useful
> a cookie as the pid. Noticed while typing docs for drm_file and friends.
> 
> Since the only consumer of this is the tracepoints I think we can safely
> change this - those tracepoints should not be uapi relevant at all. It
> all goes back to
> 
> commit b9c2c9ae882f058084e13e339925dbf8d2d20271
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Thu Jul 1 16:48:09 2010 -0700
> 
>     drm: add per-event vblank event trace points
> 
> which doesn't give a special justification for using pid over a pointer.

Well, it's friendlier to look at, I suppose.

> 
> Also note that the nouveau code setting it is entirely pointless:
> Since this isn't a vblank event, it will never hit the vblank
> tracepoints.
> 
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c              |  5 ++---
>  drivers/gpu/drm/drm_trace.h            | 20 ++++++++++----------
>  drivers/gpu/drm/nouveau/nouveau_usif.c |  1 -
>  include/drm/drm_file.h                 |  2 --
>  4 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 1906723af389..9bdca69f754c 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -978,7 +978,7 @@ static void send_vblank_event(struct drm_device *dev,
>  	e->event.tv_sec = now->tv_sec;
>  	e->event.tv_usec = now->tv_usec;
>  
> -	trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
> +	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe,
>  					 e->event.sequence);
>  
>  	drm_send_event_locked(dev, &e->base);
> @@ -1505,7 +1505,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	}
>  
>  	e->pipe = pipe;
> -	e->base.pid = current->pid;

Do you think it would be worthwhile to output the pid:file_priv mapping here in
case someone is using pid?

Regardless, the code looks correct, and I don't any skin in this game, so I'll
add my R-b and let you decide what to do if no one else complains.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>  	e->event.base.type = DRM_EVENT_VBLANK;
>  	e->event.base.length = sizeof(e->event);
>  	e->event.user_data = vblwait->request.signal;
> @@ -1534,7 +1533,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	DRM_DEBUG("event on vblank count %u, current %u, crtc %u\n",
>  		  vblwait->request.sequence, seq, pipe);
>  
> -	trace_drm_vblank_event_queued(current->pid, pipe,
> +	trace_drm_vblank_event_queued(file_priv, pipe,
>  				      vblwait->request.sequence);
>  
>  	e->event.sequence = vblwait->request.sequence;
> diff --git a/drivers/gpu/drm/drm_trace.h b/drivers/gpu/drm/drm_trace.h
> index ce3c42813fbb..14c5a777682e 100644
> --- a/drivers/gpu/drm/drm_trace.h
> +++ b/drivers/gpu/drm/drm_trace.h
> @@ -24,36 +24,36 @@ TRACE_EVENT(drm_vblank_event,
>  );
>  
>  TRACE_EVENT(drm_vblank_event_queued,
> -	    TP_PROTO(pid_t pid, int crtc, unsigned int seq),
> -	    TP_ARGS(pid, crtc, seq),
> +	    TP_PROTO(struct drm_file *file, int crtc, unsigned int seq),
> +	    TP_ARGS(file, crtc, seq),
>  	    TP_STRUCT__entry(
> -		    __field(pid_t, pid)
> +		    __field(struct drm_file *, file)
>  		    __field(int, crtc)
>  		    __field(unsigned int, seq)
>  		    ),
>  	    TP_fast_assign(
> -		    __entry->pid = pid;
> +		    __entry->file = file;
>  		    __entry->crtc = crtc;
>  		    __entry->seq = seq;
>  		    ),
> -	    TP_printk("pid=%d, crtc=%d, seq=%u", __entry->pid, __entry->crtc, \
> +	    TP_printk("file=%p, crtc=%d, seq=%u", __entry->file, __entry->crtc, \
>  		      __entry->seq)
>  );
>  
>  TRACE_EVENT(drm_vblank_event_delivered,
> -	    TP_PROTO(pid_t pid, int crtc, unsigned int seq),
> -	    TP_ARGS(pid, crtc, seq),
> +	    TP_PROTO(struct drm_file *file, int crtc, unsigned int seq),
> +	    TP_ARGS(file, crtc, seq),
>  	    TP_STRUCT__entry(
> -		    __field(pid_t, pid)
> +		    __field(struct drm_file *, file)
>  		    __field(int, crtc)
>  		    __field(unsigned int, seq)
>  		    ),
>  	    TP_fast_assign(
> -		    __entry->pid = pid;
> +		    __entry->file = file;
>  		    __entry->crtc = crtc;
>  		    __entry->seq = seq;
>  		    ),
> -	    TP_printk("pid=%d, crtc=%d, seq=%u", __entry->pid, __entry->crtc, \
> +	    TP_printk("file=%p, crtc=%d, seq=%u", __entry->file, __entry->crtc, \
>  		      __entry->seq)
>  );
>  
> diff --git a/drivers/gpu/drm/nouveau/nouveau_usif.c b/drivers/gpu/drm/nouveau/nouveau_usif.c
> index afbdbed1a690..9dc10b17ad34 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_usif.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_usif.c
> @@ -211,7 +211,6 @@ usif_notify_get(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
>  		goto done;
>  	ntfy->p->base.event = &ntfy->p->e.base;
>  	ntfy->p->base.file_priv = f;
> -	ntfy->p->base.pid = current->pid;
>  	ntfy->p->e.base.type = DRM_NOUVEAU_EVENT_NVIF;
>  	ntfy->p->e.base.length = sizeof(ntfy->p->e.base) + ntfy->reply;
>  
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index d1a25cc17fd1..4e347399a7bd 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -75,8 +75,6 @@ struct drm_pending_event {
>  	struct list_head link;
>  	struct list_head pending_link;
>  	struct drm_file *file_priv;
> -	pid_t pid; /* pid of requester, no guarantee it's valid by the time
> -		      we deliver the event, for tracing only */
>  };
>  
>  /** File private data */
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel