On Wed, Jul 15, 2015 at 11:30:13AM +0000, Gupta, Sourab wrote:
> On Wed, 2015-07-15 at 09:40 +0000, Chris Wilson wrote:
> > > static void gen_pmu_flush_snapshots(struct drm_i915_private *dev_priv)
> > > {
> > > WARN_ON(!dev_priv->gen_pmu.buffer.addr);
> > >
> > > - /* TODO: routine for forwarding snapshots to userspace */
> > > + schedule_work(&dev_priv->gen_pmu.work_timer);
> >
> > Why are we scheduling a timer? Might be a bad name for a work item to
> > infer that it is a timer.
> >
> Well, this is a case of bad name and it's a work item really.
>
> > Why is this in a work queue if you already blocked during the flush?
> >
>
> Actually, the gen_pmu_flush_snapshots() fn is called from 3 places:
> hrtimer, event_stop and event_flush. And we can block only from event
> flush. So, this function not really tied with event flush only.
>
> Also, the job of forwarding samples needs mutex to be taken (to
> dereference the request : i915_gem_request_unreference_unlocked), and
> since the forwarding has to be initiated from atomic context (e.g.
> hrtimer), I created a wq for the same, with the intention of not having
> code duplication for event flush.
>
> Probably, the better sense would be to have the forwarding functionality
> implemented in a seperate function. The work item (which would be
> scheduled from hrtimer/event stop) would be calling that function. And
> the the event flush would directly call this forwarding fn, without the
> extraneous work item scheduled. Does this seem ok?
Indeed that's an improvement for flush, where the semantics may be such
that after calling it we do expect to be able to process the samples (in
userspace) immediately.
Having a comment at the top the forward function would also help the
reader to understand the contexts from which we can be called (and why).
It's something we often lack, but is very useful for review.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
