[Intel-gfx] [PATCH 07/19] drm/i915: add runtime put/get calls at the basic places

[Chris Wilson]

On Mon, Nov 25, 2013 at 06:55:52PM -0200, Paulo Zanoni wrote:
> 2013/11/21 Chris Wilson <chris at chris-wilson.co.uk>:
> > On Thu, Nov 21, 2013 at 01:47:21PM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>
> >> If I add code to enable runtime PM on my Haswell machine, start a
> >> desktop environment, then enable runtime PM, these functions will
> >> complain that they're trying to read/write registers while the
> >> graphics card is suspended.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem.c            | 53 +++++++++++++++++++-----------
> >>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  6 ++++
> >>  drivers/gpu/drm/i915/i915_irq.c            |  6 ++++
> >>  3 files changed, 46 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index 40d9dcf..94c2a38 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -1377,36 +1377,38 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> >>       drm_i915_private_t *dev_priv = dev->dev_private;
> >>       pgoff_t page_offset;
> >>       unsigned long pfn;
> >> -     int ret = 0;
> >> +     int rc = 0, ret;
> >
> > Ugh. Just keep ret and don't add rc.
> 
> My idea was that "rc" would contain the return codes for the functions
> we call, and "ret" would contain the value we want to return. With
> this, at the end of the function we "switch (rc)" and then decide what
> we'll assign to "ret". IMHO it's confusing to mix both: we'll "switch
> (ret)" and then assign new values to "ret" inside the switch
> statement. Also, we don't have to worry about mixing values like
> EAGAIN and VM_FAULT_SIGBUS on the same variable. But I'll do the
> change, no problem: the commit diff looks much simpler with the change
> you proposed.

The way I think about it is that this function is exceptional. We
typically use a common error code that we percolate all the way back
through the stack, and so ret = callee(); if (ret) return ret; is a
clean idiom. The difference here is just that we need to translate from
one enum to another, rather like a ERR_PTR() cast. Instead of a tidy
inline function, we have the conversion as an explicit switch, but if
you think of it just as return ERR_FAULT(ret), you can see how it fits
into our typical idiom.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre