[Mesa-dev] [PATCH V2 22/22] i965/gen9: Disable Mip Tail for YF/YS tiled surfaces

[Anuj Phogat]

On Thu, May 21, 2015 at 4:55 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Fri, Apr 17, 2015 at 04:51:43PM -0700, Anuj Phogat wrote:
>> This fixed the buffer corruption happening in a FBO which use YF/YS
>> tiled renderbuffer or texture as color attachment.
>>
>> Spec recommends disabling mip tails for non-mip-mapped surfaces.
>> But, with this enabled I couldn't get correct data out of YF/YS
>> tiled surface. I get the expected data with this disabled.
>
> The docs do say to disable "to disable the Mip Tail this field must be set to a
> mip that larger than those present in the surface" So I don't see why you said,
> "But". The docs also say you must set this field when trmode isn't None.
>
> In other words, if you don't want to use mip tails, you are using the correct
> mechanism to disable it.
>
I agree I'm using the correct way of disabling it. But my point in above
comment is that there is no restriction in the docs saying "always
disable the miptails". There might be some advantage of enabling them
"by setting value < 15" which I don't understand currently. Maybe I need
to bump up my comment.

>>
>> I haven't spent any time trying to understand miptails. So, I'm
>> not sure if this patch is the right thing to do. But, It helps
>> move things forward at the moment.
>
> I don't understand them either, but I think your code is fine. Maybe add an
> assert that max level is < 15?
>
> So with some explanation in the commit message that this disables mip tails,
> this is:
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
>
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_defines.h        |  3 +++
>>  src/mesa/drivers/dri/i965/gen8_surface_state.c | 10 ++++++++--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
>> index c62c09b..1c50172 100644
>> --- a/src/mesa/drivers/dri/i965/brw_defines.h
>> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
>> @@ -594,6 +594,9 @@
>>  #define GEN9_SURFACE_TRMODE_TILEYF         1
>>  #define GEN9_SURFACE_TRMODE_TILEYS         2
>>
>> +#define GEN9_SURFACE_MIP_TAIL_START_LOD_SHIFT      8
>> +#define GEN9_SURFACE_MIP_TAIL_START_LOD_MASK       INTEL_MASK(11, 8)
>> +
>>  /* Surface state DW6 */
>>  #define GEN7_SURFACE_MCS_ENABLE                 (1 << 0)
>>  #define GEN7_SURFACE_MCS_PITCH_SHIFT            3
>> diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
>> index 189f1db..155563f 100644
>> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
>> @@ -258,8 +258,11 @@ gen8_update_texture_surface(struct gl_context *ctx,
>>                         GEN7_SURFACE_MIN_LOD) |
>>               (intelObj->_MaxLevel - tObj->BaseLevel); /* mip count */
>>
>> -   if (brw->gen >= 9)
>> +   if (brw->gen >= 9) {
>>        surf[5] |= SET_FIELD(tr_mode, GEN9_SURFACE_TRMODE);
>> +      /* Disable Mip Tail by setting a large value. */
>> +      surf[5] |= SET_FIELD(15, GEN9_SURFACE_MIP_TAIL_START_LOD);
>> +   }
>>
>>     if (aux_mt) {
>>        surf[6] = SET_FIELD(mt->qpitch / 4, GEN8_SURFACE_AUX_QPITCH) |
>> @@ -446,8 +449,11 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
>>
>>     surf[5] = irb->mt_level - irb->mt->first_level;
>>
>> -   if (brw->gen >= 9)
>> +   if (brw->gen >= 9) {
>>        surf[5] |= SET_FIELD(tr_mode, GEN9_SURFACE_TRMODE);
>> +      /* Disable Mip Tail by setting a large value. */
>> +      surf[5] |= SET_FIELD(15, GEN9_SURFACE_MIP_TAIL_START_LOD);
>> +   }
>>
>>     if (aux_mt) {
>>        surf[6] = SET_FIELD(mt->qpitch / 4, GEN8_SURFACE_AUX_QPITCH) |
>> --
>> 2.3.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> --
> Ben Widawsky, Intel Open Source Technology Center