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

[Abdiel Janulgue <abdiel.janulgue@linux.intel.com>]

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@intel.com>)
>>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@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?

> 
> 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.

Anyway, this seems to work perfectly fine in HSW as well. I'll include
that in the next version.

> 
> Note that intel_lrc.c has duplicate functions for gen8. :|
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx