[Intel-gfx] [PATCH 00/13] drm-intel-collector - review request

[Chris Wilson]

On Tue, Sep 24, 2013 at 12:47:07PM +0200, Daniel Vetter wrote:
> On Tue, Sep 24, 2013 at 12:32 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >> > Patch 04/13 - f648dba drm/i915: Asynchronously perform the set-base for a simple modeset - Reviewer:
> >>
> >> Dunno what to do exactly with this, given that the android guys seem to
> >> need more vblank waits in the plane ioctls. I'd drop for now, we need a
> >> more coherent story here ... Or just wait for nuclear flips.
> >
> > That is irrevelant for that patch. Try it, see how much it improves
> > switching between fb/X.
> 
> Well if we end up with some userspace rendering garbage then we can't
> do it ... And I somewhat suspect that the vblank waits in the android
> patch are there exactly for that reason.

But we shouldn't even be doing a set-base if we have secondary planes
that need interaction if the primary changes, hence my claims for
irrelevance. (set-base does not do any checks for secondary planes.)
Furthermore, we need to review allowing pagefips for the primary if that
causes glitches on secondary planes. Or just line up the hw designers
alongside the bios writers.

> >> > Patch 06/13 - 3b2a43a drm/i915: Pair seqno completion tracepoint with its dispatch - Reviewer:
> >>
> >> There's some bikeshed pending from Thomas Gleixner in the same are to move
> >> the i915_ring_get_seqno out of the tracepoint. I think if we frob this we
> >> might as well do it right.
> >
> > Hah, disagree completely there since that is pessimising the common
> > case. There is a reason why the expensive operation should only be done
> > when the tracepoint is active and if he wants to rewrite the tracepoint
> > API then he is welcome to submit patches...
> 
> The idea that was floated was to use the reg/unreg functions of
> DEFINE_TRACE_FN to do the irq get/put dance. The problem seems to be
> that doing fancy stuff from within the tracepoint itself isn't awesome
> for -rt locking. So we don't enable the interrupts when we don't need
> them, but only when the tracepoint is active.

And besides the complaint was about trace_irq_seqno, was it not?

> The slight problem is that I don't see any users of this stuff, so I
> think we can still go meh.

Indeed, I do not know how to do what he wants under the confines of the
current tracepoint API.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre