[Intel-gfx] [PATCH 2/2] drm/i915: Do not enable package C8 on unsupported hardware

[Chris Wilson]

On Mon, Oct 28, 2013 at 02:55:02PM -0200, Paulo Zanoni wrote:
> 2013/10/10 Chris Wilson <chris at chris-wilson.co.uk>:
> > On Thu, Oct 10, 2013 at 05:17:31PM -0300, Paulo Zanoni wrote:
> >> 2013/10/10 Chris Wilson <chris at chris-wilson.co.uk>:
> >> > If the hardware does not support package C8, then do not even schedule
> >> > work to enable it. Thereby we can eliminate a bunch of dangerous work.
> >>
> >> As I already explained, this should not be a problem since non-Haswell
> >> platforms don't have a way to make the refcount become zero (unless we
> >> have a bug). I also asked people's opinions about this specific
> >> decision in one of my cover letters, but no one said anything:
> >> http://lists.freedesktop.org/archives/intel-gfx/2013-August/031440.html.
> >>
> >> Quoting the email: "Another thing worth mentioning is that all this
> >> code doesn't have IS_HASWELL checks, and on non-Haswell platforms the
> >> refcount will never reach 0, so we won't ever try to enable PC8. I'm
> >> not sure if that's what we want, so please comment on that.".
> >>
> >> That said, I'm not against your changes.
> >
> > If they don't actually fix anything, they are low priority as they only
> > remove a mutex lock at most 10Hz. Maybe a comment would be good to remind
> > the next person that nothing gets enabled except on hsw.
> >
> >> > +#define HAS_PC8(dev)           (IS_HASWELL(dev)) /* XXX HSW:ULX */
> >>
> >> What exactly do you mean with this comment? Did you actually mean
> >> "IS_ULT()"? Even though only ULT has PC8-10 residencies, non-ULT seems
> >> to work fine with this code, so I thought it wouldn't be a problem.
> >
> > It means I didn't actually check the valid restrictions :)
> 
> Do we have plans for a V2 based on the comments? I wanted to use the
> macro on a bug fix and discovered we didn't merge this yet.

I was hoping you would take the hint and fix it up in an authorative
manner...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre