[Intel-gfx] [PATCH 2/2] drm/i915/hsw/bdw: Enable resource streamer bits on MI_BATCH_BUFFER_START

[Chris Wilson]

On Fri, May 15, 2015 at 12:16:06PM +0300, Abdiel Janulgue wrote:
> Hi,
> 
> On 05/13/2015 01:22 PM, Chris Wilson wrote:
> > On Wed, May 13, 2015 at 11:13:24AM +0300, Abdiel Janulgue wrote:
> >> Adds support for executing the resource streamer on BDW and HSW
> >>
> >> v2: Add support for Execlists (Minu Mathai <minu.mathai at intel.com>)
> >>
> >> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> > 
> > I would have liked to have seen the comments for HSW_CTX_TOTAL_SIZE
> > updated to include the resource streamer requirements.
> 
> Comment block mention HSW_CTX_TOTAL_SIZE as 70720 bytes which already
> implicitly include RS dwords.
> I can add "70720 bytes include resource streamer dwords" to the comments
> or do you want me to break this down further still?

I am just unnerved by the "full size is 70720, but we use 66944 bytes".
So perhaps changing the wording there to illustrate the boundaries would
be useful. However, I would be happy with just changing the wording to 
"...so the final size, including the extra state required for the
Resource Streamer, is 66944 bytes"

The important part here (for me at least) is that we do have something
in the git history to double check later on (i.e. we know that when we
did the RS enabling, that we did check the context size). Having clear
comments is a boon.

> > Also
> > 
> > /* These flags are for resource streamer on HSW+ */
> > if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> > 	flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > 
> > implies to me that we should be setting something for hsw to
> > save/restore RS that we do not currently. So either the comment needs
> > fixing, or we have a lack of code.
> 
> I checked, the current code does disable context save and restore for RS
> in HSW (commit e80f14b6d36e3e07111cf2ab084ef8dd5d015ce2)
> 
> But I've now modified this to:
> 
> if (IS_HASWELL(ring->dev))
> 	flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> 
> 
> so that MI_SET_CONTEXT includes RS context on HSW.

Now that makes more sense, I'd been puzzled by that comment and line of
code for ages :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre