Re: [Mesa-dev] [PATCH v2] i965: don't fail to shift height images for levels.

[Jason Ekstrand]

On Aug 23, 2016 10:10 AM, "Ilia Mirkin"  wrote:
>
> On Tue, Aug 23, 2016 at 12:34 PM, Jason Ekstrand 
wrote:
> > On Tue, Aug 23, 2016 at 9:28 AM, Jason Ekstrand 
> > 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 
> >> wrote:
> >>>
> >>> From: Dave Airlie 
> >>>
> >>> 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 
> >>> Signed-off-by: Antia Puentes 
> >>> ---
> >>>
> >>> 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

Re: [Mesa-dev] [PATCH v2] i965: don't fail to shift height images for levels.

[Ilia Mirkin]

On Tue, Aug 23, 2016 at 12:34 PM, Jason Ekstrand  wrote:
> On Tue, Aug 23, 2016 at 9:28 AM, Jason Ekstrand 
> 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 
>> wrote:
>>>
>>> From: Dave Airlie 
>>>
>>> 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 
>>> Signed-off-by: Antia Puentes 
>>> ---
>>>
>>> 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
and ms/ms_array

[this is why != 1d_array tends to be attractive]

>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

Re: [Mesa-dev] [PATCH v2] i965: don't fail to shift height images for levels.

[Jason Ekstrand]

On Tue, Aug 23, 2016 at 9:28 AM, Jason Ekstrand 
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 
> wrote:
>
>> From: Dave Airlie 
>>
>> 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 
>> Signed-off-by: Antia Puentes 
>> ---
>>
>> 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:
   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

Re: [Mesa-dev] [PATCH v2] i965: don't fail to shift height images for levels.

[Jason Ekstrand]

This smells like fish... I'm going to have a look.

On Tue, Aug 23, 2016 at 8:29 AM, Antia Puentes  wrote:

> From: Dave Airlie 
>
> 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 
> Signed-off-by: Antia Puentes 
> ---
>
> 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;
> --
> 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] [PATCH v2] i965: don't fail to shift height images for levels.

[Antia Puentes]

From: Dave Airlie 

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 
Signed-off-by: Antia Puentes 
---

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;
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev