Re: [PATCH 01/11] drm/i915: Accurately track when we mark the hardware as idle/busy

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 21, 2014 at 04:34:29PM -0300, Paulo Zanoni wrote:
> 2014-02-21 14:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Fri, Feb 21, 2014 at 02:04:32PM -0300, Paulo Zanoni wrote:
> >> 2014-02-21 13:55 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> >> > On Fri, Feb 21, 2014 at 01:52:18PM -0300, Paulo Zanoni wrote:
> >> >> From: Chris Wilson <chris@chris-wilson.co.uk>
> >> >>
> >> >> We currently call intel_mark_idle() too often, as we do so as a
> >> >> side-effect of processing the request queue. However, we the calls to
> >> >> intel_mark_idle() are expected to be paired with a call to
> >> >> intel_mark_busy() (or else we try to idle the hardware by accessing
> >> >> registers that are already disabled). Make the idle/busy tracking
> >> >> explicit to prevent the multiple calls.
> >> >>
> >> >> v2: From Paulo
> >> >>   - Make it compile
> >> >>   - Drop the __i915_add_request chunk
> >> >>
> >> >> Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_drv.h      | 8 ++++++++
> >> >>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
> >> >>  2 files changed, 17 insertions(+)
> >> >>
> >> >>
> >> >> Chris did not reply to my review comments yet, so I just went and implemented
> >> >> them. We need at least an ACK form him here before merging.
> >> >
> >> > Didn't see them... Why have you altered the logic?
> >>
> >> See the comment at the __i915_add_request chunk:
> >>
> >> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040334.html
> >
> > Oh, I didn't look for comments inline.
> >>
> >> Maybe I just broke your patch :)
> >> If my review doesn't make sense, we can stick to your version, it
> >> should do the job, and I can retest everything easily.
> >
> > If there was a pending work item, the call to intel_mark_busy() would
> > return false. So we can revamp the logic around there a little bit. The
> > reason for the change should be self-evident - the previous code lost its
> > way in the transition to multiple rings arguing over a global property
> 
> Just to avoid any possible confusions when/if we merge this series:
> Chris sent a new version of this patch on the original mail thread.

Just to double check: Have I merged the right version?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch