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

[Daniel Vetter]

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.

>> > Patch 05/13 - ec31a06 drm/i915: Align tiled scanouts from stolen memory to 256k in the GTT - Reviewer:
>>
>> I think scanouts in stolen have died for now, too many issues. So I guess
>> we can drop this, since there are other issues.
>>
>> > 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.

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

>> > Patch 07/13 - 4d98ddd drm/i915: Initialise min/max frequencies before updating RPS registers - Reviewer:
>>
>> Ville had some bikesheds pending, and there's the issue of the delayed rps
>> work. I'd drop and wait for a new version.
>
> No, he didn't. The bikesheds were not on the patch but on the
> surrounding code.

I'd still like to have an overall solution here ;-)

>> > Patch 08/13 - 35983bd drm/i915: Delay the relase of the forcewake by a jiffie - Reviewer:
>>
>> Already reviewed by Ville and needs a bit work it seems, you can drop and
>> wait for a new version.
>
> The version I (thought I) sent last has reviewed-by tags. It shouldn't
> be dropped.

Oops, indeed. Rodrigo please double-check that you still have the
latest version of patches when resending. Patch is now applied.

>> > Patch 09/13 - 14141ee drm/i915: Add a tracepoint for using a semaphore - Reviewer:
>>
>> Ville.
>>
>> > Patch 10/13 - 06d2851 drm/i915: Boost RPS frequency for CPU stalls - Reviewer:
>> > Patch 11/13 - e64dce9 drm/i915: Tweak RPS thresholds to more aggressively downclock - Reviewer:
>>
>> It sounds like Chris has updated versions of these somewhere. Imo you can
>> drop these here. Also Jesse should take a look.
>
> These seriously need review, they make a dramatic improvement to certain
> benchmarks, and have a clear impact on user experience.

Agreed. I think Jesse is volunteered to play with this stuff ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch