On Thu, Jun 11, 2015 at 9:04 AM, Ben Widawsky <b...@bwidawsk.net> wrote:
> On Wed, Jun 10, 2015 at 05:01:44PM -0700, Nanley Chery wrote:
>> From: Nanley Chery <nanley.g.ch...@intel.com>
>>
>> On Gen9+, vertical and horizontal alignment values for compressed textures
>> are
>> equal to the pre-Gen9 value squared. Each miplevel must be aligned to this
>> value.
>>
>> Signed-off-by: Nanley Chery <nanley.g.ch...@intel.com>
>
> While not a requirement, for future reference you should add Cc on patches
> which
> are touching something someone recently changed/and or was working on. In this
> case Ccing Neil would have been great.
>
I thought about this after sending out the patch unfortunately, thanks
for the reminder.
>> ---
>>
>> This fixes an FXT1 Piglit test regression and shows no failures on Jenkins.
>
> You ran full piglit on SKL? Jenkins won't and it's pretty important for this
> patch.
>
I recently ran piglit with:
./piglit run gpu -x glx -gx glean -x fbo-depth-array. There are no
regressions and 9 additional fixes. Unfortunately, my Piglit tests
haven't been consistent, so please take the results with a grain of
salt.
>>
>> src/mesa/drivers/dri/i965/brw_tex_layout.c | 20 ++++++++------------
>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> index 312a887..dffc699 100644
>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> @@ -149,15 +149,8 @@ intel_horizontal_texture_alignment_unit(struct
>> brw_context *brw,
>> unsigned int i, j;
>> _mesa_get_format_block_size(mt->format, &i, &j);
>>
>> - /* On Gen9+ we can pick our own alignment for compressed textures but
>> it
>> - * has to be a multiple of the block size. The minimum alignment we
>> can
>> - * pick is 4 so we effectively have to align to 4 times the block
>> - * size
>> - */
>> - if (brw->gen >= 9)
>> - return i * 4;
>> - else
>> - return i;
>> + /* On Gen9+ the alignment value is squared. */
>> + return brw->gen >= 9 ? i * i : i;
>
> I don't think this is right. Isn't this going to push non compressed textures
> to
> an invalid HALIGN when we divide later ie. don't you get 1?
>
The divide only occurs for compressed textures.
>> }
>>
>> if (mt->format == MESA_FORMAT_S_UINT8)
>> @@ -269,9 +262,12 @@ intel_vertical_texture_alignment_unit(struct
>> brw_context *brw,
>> * Where "*" means either VALIGN_2 or VALIGN_4 depending on the setting
>> of
>> * the SURFACE_STATE "Surface Vertical Alignment" field.
>> */
>> - if (_mesa_is_format_compressed(mt->format))
>> + if (_mesa_is_format_compressed(mt->format)) {
>> + unsigned int i, j;
>> + _mesa_get_format_block_size(mt->format, &i, &j);
>> /* See comment above for the horizontal alignment */
>
> I think you need to kill this comment now. Doesn't it refer to what you killed
> above?
>
I replaced the comment with another, so it's still valid. I can just
replace it with the same one-liner though.
>> - return brw->gen >= 9 ? 16 : 4;
>> + return brw->gen >= 9 ? j * j : j;
>> + }
>
> It kind of looks like this is just working around the way Neil implemented the
> divide later on. There's probably a nicer way to do the right thing, but I
> haven't tried it myself. Also, I believe this doesn't actually fix anything,
> it
> just uses slightly more optimal aligns - ie. maybe do a separate patch. No big
> deal though.
>
The logic is derived from the Bspec. After doing more testing and
getting Neil's feedback, I'm not completely sure the alignment is
correct. I'm looking into it more.
>>
>> if (mt->format == MESA_FORMAT_S_UINT8)
>> return brw->gen >= 7 ? 8 : 4;
>> @@ -379,7 +375,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>>
>> if (mt->compressed) {
>> mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
>> - ALIGN(minify(mt->physical_width0, 2), bw);
>> + ALIGN(minify(mt->physical_width0, 2), mt->align_w);
>> } else {
>> mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
>> minify(mt->physical_width0, 2);
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
