On Tue, Oct 20, 2015 at 12:56 AM, Pohjolainen, Topi <topi.pohjolai...@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.pho...@gmail.com> wrote: >> > Vertical alignment is not applicable to 1D textures. >> > >> > Signed-off-by: Anuj Phogat <anuj.pho...@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.pohjolai...@intel.com>
I'll make suggested changes before pushing upstream. Thanks for the review Topi. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev