On Aug 23, 2016 10:10 AM, "Ilia Mirkin" <imir...@alum.mit.edu> wrote:
>
> On Tue, Aug 23, 2016 at 12:34 PM, Jason Ekstrand <ja...@jlekstrand.net>
wrote:
> > On Tue, Aug 23, 2016 at 9:28 AM, Jason Ekstrand <ja...@jlekstrand.net>
> > wrote:
> >>
> >> This smells like fish... I'm going to have a look.
> >
> >
> > Ok, I looked...
> >
> >>
> >> On Tue, Aug 23, 2016 at 8:29 AM, Antia Puentes <apuen...@igalia.com>
> >> wrote:
> >>>
> >>> From: Dave Airlie <airl...@redhat.com>
> >>>
> >>> This fixes one subtest of:
> >>> GL44-CTS.shader_image_size.advanced-nonMS-fs-int
> >>>
> >>> I've no idea why this wouldn't be scaled up here,
> >>> and I've no idea what else will break, but I might
> >>> as well open for discussion.
> >>>
> >>> v2: Only shift height if the texture is not an 1D_ARRAY,
> >>> it fixes assertion in GL44-CTS.texture_view.gettexparameter
> >>> due to the original patch (Antia).
> >>>
> >>> Signed-off-by: Dave Airlie <airl...@redhat.com>
> >>> Signed-off-by: Antia Puentes <apuen...@igalia.com>
> >>> ---
> >>>
> >>> I have not taken a deep look to the test so take this with a grain of
> >>> salt.
> >>> As I said in a previous email, this patch raises an assertion in
> >>> GL44-CTS.texture_view.gettexparameter:
> >>>
> >>> "glcts: intel_mipmap_tree.c:368: intel_miptree_create_layout:
Assertion
> >>> `height0 = 1' failed."
> >>>
> >>> Looking at the code surrounding the assertion, we have:
> >>>
> >>> if (target == GL_TEXTURE_1D_ARRAY)
> >>> assert(height0 == 1);
> >>>
> >>> which suggests that we should avoid shifting the height at least for
> >>> TEXTURE_1D_ARRAYs. Sending a second version of the patch.
> >>>
> >>> src/mesa/drivers/dri/i965/intel_tex_image.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c
> >>> b/src/mesa/drivers/dri/i965/intel_tex_image.c
> >>> index 958f8bd..120e7e0 100644
> >>> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> >>> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> >>> @@ -49,7 +49,7 @@ intel_miptree_create_for_teximage(struct brw_context
> >>> *brw,
> >>> /* Figure out image dimensions at start level. */
> >>> for (i = intelImage->base.Base.Level; i > 0; i--) {
> >>> width <<= 1;
> >>> - if (height != 1)
> >>> + if (intelObj->base.Target != GL_TEXTURE_1D_ARRAY)
> >>> height <<= 1;
> >>> if (intelObj->base.Target == GL_TEXTURE_3D)
> >>> depth <<= 1;
> >
> >
> > I think the whole pile of code is bogus and needs to be rewritten.
First
> > off, why are we using a for loop? Seriously? Second, I think what we
want
> > is more like
> >
> > switch (intelObj->base.Target) {
> > case GL_TEXTURE_3D:
> > depth <<= intelImage->base.Base.Level;
> > /* Fall through */
> >
> > case GL_TEXTURE_2D:
> > case GL_TEXTURE_2D_ARRAY:
> > case GL_TEXTURE_RECTANGLE:
>
> and cube/cube_array
Yes
> and ms/ms_array
Noish... You can't have multiple levels for multisampled but sure.
> [this is why != 1d_array tends to be attractive]
Sure, but it had better be != 1d &&!= 1d_array. I'm not married to the
switch statement, but we could easily make it exhaustive with an
unreachable default case and solve that problem. That may actually be
better than != 1d. If we did that, we could put an assert(levels ==0) in
the case for multisampled.
In any case, I do think we should kill the loop and be a bit more explicit.
--Jason
> > height <<= intelImage->base.Base.Level
> > /* Fall through */
> >
> > default:
> > width <<= intelImage->base.Base.Level;
> > }
> >
> > I think that would be far more clear and correct. I don't have a build
of
> > the CTS handy so I didn't send it as a 3rd counter-patch.
> >
> > --Jason
> >
> >>>
> >>> --
> >>> 2.7.4
> >>>
> >>> _______________________________________________
> >>> mesa-dev mailing list
> >>> mesa-dev@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>
> >>
> >
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
