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

[Jason Ekstrand]

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,

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

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