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

[Pohjolainen, Topi]

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>