[PATCH 5/9] drm/i915: Enable i915 perf stream for Haswell OA unit

[Chris Wilson]

On Thu, Apr 21, 2016 at 04:43:19PM +0100, Robert Bragg wrote:
>    On Wed, Apr 20, 2016 at 11:52 PM, Chris Wilson
>    <[1]chris at chris-wilson.co.uk> wrote:
> 
>      On Wed, Apr 20, 2016 at 03:23:10PM +0100, Robert Bragg wrote:
>      > +static int i915_oa_read(struct i915_perf_stream *stream,
>      > +                     struct i915_perf_read_state *read_state)
>      > +{
>      > +     struct drm_i915_private *dev_priv = stream->dev_priv;
>      > +
>      > +     return dev_priv->perf.oa.ops.read(stream, read_state);
>      > +}
> 
>      > +     stream->destroy = i915_oa_stream_destroy;
>      > +     stream->enable = i915_oa_stream_enable;
>      > +     stream->disable = i915_oa_stream_disable;
>      > +     stream->can_read = i915_oa_can_read;
>      > +     stream->wait_unlocked = i915_oa_wait_unlocked;
>      > +     stream->poll_wait = i915_oa_poll_wait;
>      > +     stream->read = i915_oa_read;
> 
>      Why aren't these a const ops table?
> 
>    No particular reason; I guess it just seemed straightforward enough at the
>    time. I suppose it avoids some redundant pointer indirection and could
>    suit defining streams in the future that might find it awkward to have
>    static ops (don't have anything like that in mind though) but it's at the
>    expense of a slightly larger stream struct (though also don't see that as
>    a concern currently).
> 
>    Can change if you like.

I think it is safe to say it is considered best practice to have vfunc
tables in read-only memory. Certainly raises an eyebrow when they look
like they could be modified on the fly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre