Re: [PATCH mesa v2] i965/gen8+: bo in state base address must be in 32-bit address range

[Michel Thierry <michel.thierry@intel.com>]

On 7/2/2015 8:21 AM, Chris Wilson wrote:
> On Wed, Jul 01, 2015 at 04:28:10PM +0100, Michel Thierry wrote:
>> Gen8+ supports 48-bit virtual addresses, but some objects must always be
>> allocated inside the 32-bit address range.
>>
>>      OUT_BATCH(0);
>>      OUT_BATCH(mocs_wb << 16);
>>      /* Surface state base address: */
>> -   OUT_RELOC64(brw->batch.bo, I915_GEM_DOMAIN_SAMPLER, 0,
>> -               mocs_wb << 4 | 1);
>> +   OUT_RELOC(brw->batch.bo, I915_GEM_DOMAIN_SAMPLER, 0,
>> +             mocs_wb << 4 | 1);
>> +   OUT_BATCH(0);
>>      /* Dynamic state base address: */
>> -   OUT_RELOC64(brw->batch.bo,
>> -               I915_GEM_DOMAIN_RENDER | I915_GEM_DOMAIN_INSTRUCTION, 0,
>> -               mocs_wb << 4 | 1);
>> +   OUT_RELOC(brw->batch.bo,
>> +             I915_GEM_DOMAIN_RENDER | I915_GEM_DOMAIN_INSTRUCTION, 0,
>> +             mocs_wb << 4 | 1);
>> +   OUT_BATCH(0);
>
> Note this is in general dangerous since it lies to the kernel about the
> value written into the batch corresponding to the 64bit relocation.
> (Aliasing a high object here isn't much of an issue since the relocation
> will be forced by having to rebind the buffer low.)
>
> Personally I would have stuck with the OUT_RELOC64 so that any future
> cut'n'paste didn't have a subtle bug and that the wa was clearly
> indicated by clearing the execobject flag for brw->batch.bo and
> brw->cache.bo.
>
>>      /* Indirect object base address: MEDIA_OBJECT data */
>>      OUT_BATCH(mocs_wb << 4 | 1);
>>      OUT_BATCH(0);
>>      /* Instruction base address: shader kernels (incl. SIP) */
>> -   OUT_RELOC64(brw->cache.bo, I915_GEM_DOMAIN_INSTRUCTION, 0,
>> -               mocs_wb << 4 | 1);
>> -
>> +   OUT_RELOC(brw->cache.bo, I915_GEM_DOMAIN_INSTRUCTION, 0,
>> +             mocs_wb << 4 | 1);
>> +   OUT_BATCH(0);
>>      /* General state buffer size */
>>      OUT_BATCH(0xfffff001);
>>      /* Dynamic state buffer size */
>> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> index 54081a1..220a35b 100644
>> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
>> @@ -411,9 +411,9 @@ intel_batchbuffer_emit_reloc64(struct brw_context *brw,
>>                                  uint32_t read_domains, uint32_t write_domain,
>>   			       uint32_t delta)
>>   {
>> -   int ret = drm_intel_bo_emit_reloc(brw->batch.bo, 4*brw->batch.used,
>> -                                     buffer, delta,
>> -                                     read_domains, write_domain);
>> +   int ret = drm_intel_bo_emit_reloc_48bit(brw->batch.bo, 4*brw->batch.used,
>> +                                           buffer, delta,
>> +                                           read_domains, write_domain);
>
> I would have just exposed setting the flag on the execobject.  That way you
> still have existing userspace safe by default, can set a
> bufmgr-level flag to enable 48bit support by default and then
> individually turn off 48bit support for the couple of buffers that
> require it.
>
So, something more like v1? 
http://mid.gmane.org/1435062089-19877-2-git-send-email-michel.thierry@intel.com

(apart of the hacky macro name)

> Just my 2c because I have seen what trouble lying to the kernel about
> relocation values can cause and would rather not have that in the
> interface by design.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx