[Mesa-dev] Patchwork/mesa-stable question (was: Re: [PATCH v3] r600g: Fix special negative immediate constants when using ABS modifier.)

2015-10-29 Thread Nicolai Hähnle 

On 29.10.2015 10:24, Ivan Kalvachev wrote:
[snip]

On 10/29/15, Nicolai Hähnle  wrote:

On 29.10.2015 01:52, Ivan Kalvachev wrote:

On 10/26/15, Nicolai Hähnle  wrote:

On 25.10.2015 02:00, Ivan Kalvachev wrote:

Some constants (like 1.0 and 0.5) could be inlined as immediate inputs
without using their literal value. The
r600_bytecode_special_constants()
function emulates the negative of these constants by using NEG
modifier.

However some shaders define -1.0 constant and want to use it as 1.0.
They do so by using ABS modifier. But r600_bytecode_special_constants()
set NEG in addition to ABS. Since NEG modifier have priority over ABS
one,
we get -|1.0| as result, instead of |1.0|.

The patch simply prevents the additional switching of NEG when ABS is
set.


Nice catch. Is there a simple test case (e.g. in piglit) that exposes
the incorrect behavior?


Not that I know of.

I've located the bug investigating visual problem in Nine.
https://github.com/iXit/Mesa-3D/issues/126
https://github.com/iXit/Mesa-3D/issues/127

I also heard that it fixes artifacts in "Need for Speed: Undercover"
and "Skyrim", once again, when using Nine.


I see. I guess it's not too surprising that Nine creates shaders that
look a bit different from the Mesa statetracker's.

Reviewed-by: Nicolai Hähnle 

This should probably also go to stable.

Do you need somebody to push this for you or can you do it yourself?

Cheers,
Nicolai


Yes, please.
I'm not developer and I cannot push it myself.


I pushed the patch.

I am not familiar with patchwork yet and have a related question: on my 
push, I got the following error message related to patchwork:


remote: E: failed to find patch for rev 
f75f21a24ae2dd83507f3d4d8007f0fcfe6db802


Apparently, patchwork didn't pick up Ivan's v3 patch, perhaps because it 
wasn't inline. Is this something to worry about? Specifically, I believe 
the patch is a candidate for the stable branch, and I added the 
appropriate Cc: in the commit message. Does the message above prevent it 
from being picked up?


Sorry for the noise :/

Thanks!
Nicolai
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Patchwork/mesa-stable question (was: Re: [PATCH v3] r600g: Fix special negative immediate constants when using ABS modifier.)

2015-10-29 Thread Ilia Mirkin 
On Thu, Oct 29, 2015 at 7:11 PM, Nicolai Hähnle  wrote:
> On 29.10.2015 10:24, Ivan Kalvachev wrote:
> [snip]
>>
>> On 10/29/15, Nicolai Hähnle  wrote:
>>>
>>> On 29.10.2015 01:52, Ivan Kalvachev wrote:

 On 10/26/15, Nicolai Hähnle  wrote:
>
> On 25.10.2015 02:00, Ivan Kalvachev wrote:
>>
>> Some constants (like 1.0 and 0.5) could be inlined as immediate inputs
>> without using their literal value. The
>> r600_bytecode_special_constants()
>> function emulates the negative of these constants by using NEG
>> modifier.
>>
>> However some shaders define -1.0 constant and want to use it as 1.0.
>> They do so by using ABS modifier. But
>> r600_bytecode_special_constants()
>> set NEG in addition to ABS. Since NEG modifier have priority over ABS
>> one,
>> we get -|1.0| as result, instead of |1.0|.
>>
>> The patch simply prevents the additional switching of NEG when ABS is
>> set.
>
>
> Nice catch. Is there a simple test case (e.g. in piglit) that exposes
> the incorrect behavior?


 Not that I know of.

 I've located the bug investigating visual problem in Nine.
 https://github.com/iXit/Mesa-3D/issues/126
 https://github.com/iXit/Mesa-3D/issues/127

 I also heard that it fixes artifacts in "Need for Speed: Undercover"
 and "Skyrim", once again, when using Nine.
>>>
>>>
>>> I see. I guess it's not too surprising that Nine creates shaders that
>>> look a bit different from the Mesa statetracker's.
>>>
>>> Reviewed-by: Nicolai Hähnle 
>>>
>>> This should probably also go to stable.
>>>
>>> Do you need somebody to push this for you or can you do it yourself?
>>>
>>> Cheers,
>>> Nicolai
>>
>>
>> Yes, please.
>> I'm not developer and I cannot push it myself.
>
>
> I pushed the patch.
>
> I am not familiar with patchwork yet and have a related question: on my
> push, I got the following error message related to patchwork:
>
> remote: E: failed to find patch for rev
> f75f21a24ae2dd83507f3d4d8007f0fcfe6db802
>
> Apparently, patchwork didn't pick up Ivan's v3 patch,

Now you know why there are so many stale patches in patchwork... if
the diff part of the commit doesn't match 100% to something in
patchwork, the search fails.

> perhaps because it
> wasn't inline. Is this something to worry about? Specifically, I believe the
> patch is a candidate for the stable branch, and I added the appropriate Cc:
> in the commit message. Does the message above prevent it from being picked
> up?

Nope, all's well. Assuming that the thing you wanted to get pushed has
been. Patchwork isn't part of any "official" process, just a
convenience tool.

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

Re: [Mesa-dev] Patchwork/mesa-stable question (was: Re: [PATCH v3] r600g: Fix special negative immediate constants when using ABS modifier.)

2015-10-29 Thread Matt Turner 
On Thu, Oct 29, 2015 at 4:23 PM, Ilia Mirkin  wrote:
> On Thu, Oct 29, 2015 at 7:11 PM, Nicolai Hähnle  wrote:
>> perhaps because it
>> wasn't inline. Is this something to worry about? Specifically, I believe the
>> patch is a candidate for the stable branch, and I added the appropriate Cc:
>> in the commit message. Does the message above prevent it from being picked
>> up?
>
> Nope, all's well. Assuming that the thing you wanted to get pushed has
> been. Patchwork isn't part of any "official" process, just a
> convenience tool.

While that's true, not cleaning up patches you push just makes it
harder to find unapplied patches that have fallen through the cracks.

The nice thing to do when you see that message is to mark the patch as
accepted/superseded.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev