On Tue, Aug 11, 2015 at 2:45 AM, Francisco Jerez <curroje...@riseup.net> wrote:
> Jason Ekstrand <ja...@jlekstrand.net> writes:
>
>> On Sat, Aug 8, 2015 at 2:58 AM, Francisco Jerez <curroje...@riseup.net>
>> wrote:
>>> Jason Ekstrand <ja...@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.ekstr...@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 <curroje...@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@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
