[Mesa-dev] [PATCH 1/2] i965: Add SKL support to brw_miptree_get_horizontal_slice_pitch().

[Jason Ekstrand]

On Tue, Aug 11, 2015 at 2:45 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Jason Ekstrand <jason at jlekstrand.net> writes:
>
>> On Sat, Aug 8, 2015 at 2:58 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>>
>>>> I'm not a huge fan of this patch.  Really, given how complicated 3-D
>>>> textures are on SKL, there really is no sensible horizontal slice
>>>> pitch.  We could return 0 as an "invalid value" but I think I'd rather
>>>> keep it an assert.  Code that is dealing with 3-D textures should no
>>>> better than to just blindly call this function on SKL.
>>>> --Jason
>>>
>>> How so?  The sub-tile structure may indeed be more complicated on SKL,
>>> but the way how Z-slices of whole tiles are laid out in 2D memory for 3D
>>> textures is even simpler than on earlier generations -- Say a given
>>> tile-slice of LOD l starts at 2D coordinates (x, y), the next tile-slice
>>> will start at (x, y + qpitch), IOW the horizontal offset between
>>> tile-slices is zero which is what this patch does.  We could probably
>>> keep the assertion I had but that would complicate
>>> update_texture_image_param() a bit more so I'd rather do what AFAIUI is
>>> the right thing here.
>>
>> Doing a little spelunking, it appears as if the code this patch is
>> modifying is only used for setting up image uniforms (i.e., it's dead
>> at the moment) and was pushed without review. *sigh*
>>
>> Given that, this patch certainly doesn't break anything and seems to
>> do what you claim it does so I'm fine with it.  That said, please do a
>> follow-on patch that actually documents these new tex_layout
>> entrypoints.  In particular, it should be documented what value they
>> compute/return.  The above description, re-written in a form more
>> appropriate for a comment, is probably sufficient for
>> get_horizontal_slice_pitch.  Assuming the follow-on will happen,
>>
>
> There is some documentation about them in the intel_mipmap_tree.h header
> file, let me know if it's not sufficient for you so I write the
> follow-up.

Hrm... I thought we usually put documentation in the C file; I guess
not in the case of miptree code... oh, well.

Yes, I do think it should be expanded.  Especially since the comment
explicitly mentions 2-d miptree layout but it's really for 3-d
textures. :-)
--Jason

>> Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
>>
>> Go land things!  You can also apply my R-B to the patch that enables
>> the extension (I don't want dig through my e-mail so I can respond to
>> the specific patch.)
>>
> Cool, thanks.
>
>> --Jason
>>
>>>>
>>>> On Wed, May 13, 2015 at 9:37 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>> ---
>>>>>  src/mesa/drivers/dri/i965/brw_tex_layout.c | 4 +---
>>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>>>>> index 72b02a2..6c6dc5c 100644
>>>>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>>>>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>>>>> @@ -281,9 +281,7 @@ brw_miptree_get_horizontal_slice_pitch(const struct brw_context *brw,
>>>>>                                         const struct intel_mipmap_tree *mt,
>>>>>                                         unsigned level)
>>>>>  {
>>>>> -   assert(brw->gen < 9);
>>>>> -
>>>>> -   if (mt->target == GL_TEXTURE_3D ||
>>>>> +   if ((brw->gen < 9 && mt->target == GL_TEXTURE_3D) ||
>>>>>         (brw->gen == 4 && mt->target == GL_TEXTURE_CUBE_MAP)) {
>>>>>        return ALIGN(minify(mt->physical_width0, level), mt->align_w);
>>>>>     } else {
>>>>> --
>>>>> 2.3.5
>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev