[Mesa-dev] [PATCH 14/27] i965: Allow aux buffers to have an offset

[Ben Widawsky]

On 16-12-01 14:41:20, Kenneth Graunke wrote:
>On Thursday, December 1, 2016 2:09:55 PM PST Ben Widawsky wrote:
>> From: Ben Widawsky <ben at bwidawsk.net>
>>
>> Previously our aux buffers (MCS, and HiZ) never had an offset because
>> they were in their own buffer object. When using the CCS lossless
>> compression feature, it's desirable to store the data at an offset from
>> the main framebuffer, ie. share a buffer object. This patch just makes
>> having an aux offset possible.
>>
>> Thanks to Ken for helping me find the most simple fix...
>>
>> Cc: Kenneth Graunke <kenneth at whitecape.org>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 3 +--
>>  src/mesa/drivers/dri/i965/intel_image.h          | 3 +++
>>  src/mesa/drivers/dri/i965/intel_screen.c         | 5 +++++
>>  3 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>> index 6c44381..69be3eb 100644
>> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>> @@ -143,7 +143,6 @@ brw_emit_surface_state(struct brw_context *brw,
>>        aux_surf = &aux_surf_s;
>>
>>        if (mt->mcs_buf) {
>> -         assert(mt->mcs_buf->offset == 0);
>>           aux_bo = mt->mcs_buf->bo;
>>           aux_offset = mt->mcs_buf->bo->offset64 + mt->mcs_buf->offset;
>>        } else {
>> @@ -185,7 +184,7 @@ brw_emit_surface_state(struct brw_context *brw,
>>        uint32_t *aux_addr = state + brw->isl_dev.ss.aux_addr_offset;
>>        drm_intel_bo_emit_reloc(brw->batch.bo,
>>                                *surf_offset + brw->isl_dev.ss.aux_addr_offset,
>> -                              aux_bo, *aux_addr & 0xfff,
>> +                              aux_bo, *aux_addr - aux_bo->offset64,
>>                                read_domains, write_domains);
>>     }
>>  }
>
>This seems like two separate patches - one above (fixing SURFACE_STATE
>to actually handle non-zero aux offsets), and one below (adding offset
>fields for DRI images).  Could we split it?
>
>The one above would get a:
>Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Sure I'll split it, thanks. It was logically the same in my opinion because
before we added an aux_offset, aux_addr should always have been just the low 12
bits...

>
>I'd need to read the one below in more context.
>
>> diff --git a/src/mesa/drivers/dri/i965/intel_image.h b/src/mesa/drivers/dri/i965/intel_image.h
>> index bbda952..7b3c624 100644
>> --- a/src/mesa/drivers/dri/i965/intel_image.h
>> +++ b/src/mesa/drivers/dri/i965/intel_image.h
>> @@ -92,6 +92,9 @@ struct __DRIimageRec {
>>     /** The image was created with EGL_EXT_image_dma_buf_import. */
>>     bool dma_buf_imported;
>>
>> +   /** The image has some ancillary data associated with it at offset. */
>> +   uint32_t aux_offset;
>> +
>>     /**
>>      * Provided by EGL_EXT_image_dma_buf_import.
>>      * \{
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>> index e0402cf..0f19a6e 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -572,6 +572,11 @@ create_image_with_modifier(struct intel_screen *screen,
>>     image->tile_y = I915_TILING_Y;
>>     image->modifier = modifier;
>>
>> +   if (image->planar_format)
>> +      assert(image->planar_format->nplanes == 1);
>> +
>> +   image->aux_offset = 0; /* y_tiled_height * pitch; */
>> +
>>     return true;
>>  }
>>
>>
>



-- 
Ben Widawsky, Intel Open Source Technology Center