[Mesa-dev] [PATCH 2/7] i965/gen9: Don't call tr_mode_vertical_texture_alignment() for 1D textures

[Anuj Phogat]

On Tue, Oct 20, 2015 at 12:56 AM, Pohjolainen, Topi
<topi.pohjolainen at intel.com> wrote:
> On Mon, Oct 19, 2015 at 02:29:04PM -0700, Anuj Phogat wrote:
>> On Thu, Aug 13, 2015 at 2:51 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
>> > Vertical alignment is not applicable to 1D textures.
>> >
>> > Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_tex_layout.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> > index 4e44b15..edd7518 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> > +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> > @@ -277,7 +277,9 @@ intel_vertical_texture_alignment_unit(struct brw_context *brw,
>> >     if (mt->format == MESA_FORMAT_S_UINT8)
>> >        return brw->gen >= 7 ? 8 : 4;
>> >
>> > -   if (mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE) {
>> > +   if (mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE &&
>> > +       mt->target != GL_TEXTURE_1D &&
>> > +       mt->target != GL_TEXTURE_1D_ARRAY) {
>
> There is the exact same assertion in tr_mode_vertical_texture_alignment(),
> and therefore this makes sense.
>
>> >        uint32_t align = tr_mode_vertical_texture_alignment(brw, mt);
>> >        /* XY_FAST_COPY_BLT doesn't support vertical alignment < 64 */
>> >        return align < 64 ? 64 : align;
>> > --
>> > 2.4.3
>> >
>>
>> Patches 2, 4, 5-7 of this series are waiting for review. These patches are doing
>> simple changes and should be easy to review. Here is a patchwork link to the
>> list of patches:
>> http://patchwork.freedesktop.org/project/mesa/patches/?submitter=10862
>
> Along with patch 2, number 5 is clear improvment also. The replacament of the
> static alignment table with a multiplier (targeting another alignment table)
> in patch 4 wasn't at first a clear improvement to me. But combined with
> patch 6 I can see them reducing the number of lines of code and therefore I'm
> in favor of commiting them as well.
> In patch 4 you could call "multiplier" as "multiplier_ys" as it is
> specifically used to derive YS alignment from YF alignment.
> In patch 6 (or later in patch 7) you could declare the variable "i" as const
> and initialize it right away with the correct value.
>
> Using "mt->cpp" instead of "_mesa_get_format_bytes(mt->format) * 8" in patch
> 7 looks better to me also.
>
> So 2 and 4-7 are:
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

I'll make suggested changes before pushing upstream. Thanks for the review Topi.