| Message ID | 20170308141257.12119-11-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Mon, Mar 13, 2017 at 01:05:27PM -0400, Sean Paul wrote: > 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. I considered it, but then went meh. It's super-easy to change back to fpriv->pid if anyone wants that, so if someone pipes up I'll volunteer for that. The one issue with that is that on systems with logind, logind opens all drm fd for compositors, so they all have the same pid. With fpriv itself you can at least tell them apart ... > Reviewed-by: Sean Paul <seanpaul@chromium.org> I'll take this one :-) -Daniel
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; 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 */
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. 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(-)