Re: [Mesa-dev] [PATCH] i915g: fix incorrect gl_FragCoord value

[Stéphane Marchesin]

I pushed it. Thanks Nicholas.

Stéphane


On Thu, Sep 22, 2016 at 9:40 AM, Roland Scheidegger 
wrote:

> Am 22.09.2016 um 17:40 schrieb Emil Velikov:
> > Hi Nicholas,
> >
> > On 26 August 2016 at 00:31, Nicholas Bishop 
> wrote:
> >> From: Nicholas Bishop 
> >>
> >> On Intel Pineview M hardware, the i915 gallium driver doesn't output
> >> the correct gl_FragCoord. It seems to always have an X coord of 0.0
> >> and a Y coord of the window's height in pixels, e.g. 600.0f or such.
> >>
> >> I believe this is a regression caused in part by this commit:
> >> afa035031ff9e0c07a2297d864e46c76f7bfff58
> >>
> >> The old behavior used the output at index zero, while the new behavior
> >> uses actual zeroes. In the case of gl_FragCoord the output at index
> >> zero happened to be the correct one, so the behavior appeared correct
> >> although the code already had a bug.
> >>
> >> Fixed by checking for I915_SEMANTIC_POS when setting up texCoords. If
> >> the generic_mapping is I915_SEMANTIC_POS, look for the
> >> TGSI_SEMANTIC_POSITION instead of a TGSI_SEMANTIC_GENERIC output.
> >>
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.
> freedesktop.org_show-5Fbug.cgi-3Fid-3D97477=CwIFaQ=
> Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=Vjtt0vs_
> iqoI31UfJxBl7yv9I2FeiaeAYgMTLKRBc_I=Mt7V3FvGsInekgNmMd2jOoCYRJDl4I
> f-uvNtbOhGkRQ=JlfHHRDgBQvDfacPpT-qFUmY4drEprImNSdf1L6ch3c=
> >> ---
> >>  src/gallium/drivers/i915/i915_state_derived.c | 7 ++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/gallium/drivers/i915/i915_state_derived.c
> b/src/gallium/drivers/i915/i915_state_derived.c
> >> index 177b854..dbfbc84 100644
> >> --- a/src/gallium/drivers/i915/i915_state_derived.c
> >> +++ b/src/gallium/drivers/i915/i915_state_derived.c
> >> @@ -145,7 +145,12 @@ static void calculate_vertex_layout(struct
> i915_context *i915)
> >>uint hwtc;
> >>if (texCoords[i]) {
> >>   hwtc = TEXCOORDFMT_4D;
> >> - src = draw_find_shader_output(i915->draw,
> TGSI_SEMANTIC_GENERIC, fs->generic_mapping[i]);
> >> + if (fs->generic_mapping[i] == I915_SEMANTIC_POS) {
> >> +src = draw_find_shader_output(i915->draw,
> TGSI_SEMANTIC_POSITION, 0);
> >> + }
> >> + else {
> >> +src = draw_find_shader_output(i915->draw,
> TGSI_SEMANTIC_GENERIC, fs->generic_mapping[i]);
> >> + }
> >
> > Personally I'm unfamiliar with the i915g driver, so I've Cc'd Roland
> > (the author of the commit 'breaking' this) and Stéphane (the author of
> > this driver).
> >
> > Gents, any input on the above ?
>
> Oh, I wasn't aware I broke this...
> This looks to me like before my patch there i915g was indeed just
> relying on draw code redirecting undefined inputs to 0, which near
> certainly was unspecified behavior.
> I don't really know i915g, but this looks like it should be correct.
>
> Reviewed-by: Roland Scheidegger 
>
> >
> > That said, please drop the unneeded brackets and add the following
> > line in the commit message for v2.
> > Cc: mesa-sta...@lists.freedesktop.org
> >
> > Thanks,
> > Emil
> >
>
> ___
> 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] i915g: fix incorrect gl_FragCoord value

[Roland Scheidegger]

Am 22.09.2016 um 17:40 schrieb Emil Velikov:
> Hi Nicholas,
> 
> On 26 August 2016 at 00:31, Nicholas Bishop  wrote:
>> From: Nicholas Bishop 
>>
>> On Intel Pineview M hardware, the i915 gallium driver doesn't output
>> the correct gl_FragCoord. It seems to always have an X coord of 0.0
>> and a Y coord of the window's height in pixels, e.g. 600.0f or such.
>>
>> I believe this is a regression caused in part by this commit:
>> afa035031ff9e0c07a2297d864e46c76f7bfff58
>>
>> The old behavior used the output at index zero, while the new behavior
>> uses actual zeroes. In the case of gl_FragCoord the output at index
>> zero happened to be the correct one, so the behavior appeared correct
>> although the code already had a bug.
>>
>> Fixed by checking for I915_SEMANTIC_POS when setting up texCoords. If
>> the generic_mapping is I915_SEMANTIC_POS, look for the
>> TGSI_SEMANTIC_POSITION instead of a TGSI_SEMANTIC_GENERIC output.
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D97477=CwIFaQ=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs=Vjtt0vs_iqoI31UfJxBl7yv9I2FeiaeAYgMTLKRBc_I=Mt7V3FvGsInekgNmMd2jOoCYRJDl4If-uvNtbOhGkRQ=JlfHHRDgBQvDfacPpT-qFUmY4drEprImNSdf1L6ch3c=
>>  
>> ---
>>  src/gallium/drivers/i915/i915_state_derived.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/drivers/i915/i915_state_derived.c 
>> b/src/gallium/drivers/i915/i915_state_derived.c
>> index 177b854..dbfbc84 100644
>> --- a/src/gallium/drivers/i915/i915_state_derived.c
>> +++ b/src/gallium/drivers/i915/i915_state_derived.c
>> @@ -145,7 +145,12 @@ static void calculate_vertex_layout(struct i915_context 
>> *i915)
>>uint hwtc;
>>if (texCoords[i]) {
>>   hwtc = TEXCOORDFMT_4D;
>> - src = draw_find_shader_output(i915->draw, TGSI_SEMANTIC_GENERIC, 
>> fs->generic_mapping[i]);
>> + if (fs->generic_mapping[i] == I915_SEMANTIC_POS) {
>> +src = draw_find_shader_output(i915->draw, 
>> TGSI_SEMANTIC_POSITION, 0);
>> + }
>> + else {
>> +src = draw_find_shader_output(i915->draw, 
>> TGSI_SEMANTIC_GENERIC, fs->generic_mapping[i]);
>> + }
> 
> Personally I'm unfamiliar with the i915g driver, so I've Cc'd Roland
> (the author of the commit 'breaking' this) and Stéphane (the author of
> this driver).
> 
> Gents, any input on the above ?

Oh, I wasn't aware I broke this...
This looks to me like before my patch there i915g was indeed just
relying on draw code redirecting undefined inputs to 0, which near
certainly was unspecified behavior.
I don't really know i915g, but this looks like it should be correct.

Reviewed-by: Roland Scheidegger 

> 
> That said, please drop the unneeded brackets and add the following
> line in the commit message for v2.
> Cc: mesa-sta...@lists.freedesktop.org
> 
> Thanks,
> Emil
> 

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

Re: [Mesa-dev] [PATCH] i915g: fix incorrect gl_FragCoord value

[Stéphane Marchesin]

On Thu, Sep 22, 2016 at 8:40 AM, Emil Velikov 
wrote:

> Hi Nicholas,
>
> On 26 August 2016 at 00:31, Nicholas Bishop  wrote:
> > From: Nicholas Bishop 
> >
> > On Intel Pineview M hardware, the i915 gallium driver doesn't output
> > the correct gl_FragCoord. It seems to always have an X coord of 0.0
> > and a Y coord of the window's height in pixels, e.g. 600.0f or such.
> >
> > I believe this is a regression caused in part by this commit:
> > afa035031ff9e0c07a2297d864e46c76f7bfff58
> >
> > The old behavior used the output at index zero, while the new behavior
> > uses actual zeroes. In the case of gl_FragCoord the output at index
> > zero happened to be the correct one, so the behavior appeared correct
> > although the code already had a bug.
> >
> > Fixed by checking for I915_SEMANTIC_POS when setting up texCoords. If
> > the generic_mapping is I915_SEMANTIC_POS, look for the
> > TGSI_SEMANTIC_POSITION instead of a TGSI_SEMANTIC_GENERIC output.
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=97477
> > ---
> >  src/gallium/drivers/i915/i915_state_derived.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/gallium/drivers/i915/i915_state_derived.c
> b/src/gallium/drivers/i915/i915_state_derived.c
> > index 177b854..dbfbc84 100644
> > --- a/src/gallium/drivers/i915/i915_state_derived.c
> > +++ b/src/gallium/drivers/i915/i915_state_derived.c
> > @@ -145,7 +145,12 @@ static void calculate_vertex_layout(struct
> i915_context *i915)
> >uint hwtc;
> >if (texCoords[i]) {
> >   hwtc = TEXCOORDFMT_4D;
> > - src = draw_find_shader_output(i915->draw,
> TGSI_SEMANTIC_GENERIC, fs->generic_mapping[i]);
> > + if (fs->generic_mapping[i] == I915_SEMANTIC_POS) {
> > +src = draw_find_shader_output(i915->draw,
> TGSI_SEMANTIC_POSITION, 0);
> > + }
> > + else {
> > +src = draw_find_shader_output(i915->draw,
> TGSI_SEMANTIC_GENERIC, fs->generic_mapping[i]);
> > + }
>
> Personally I'm unfamiliar with the i915g driver, so I've Cc'd Roland
> (the author of the commit 'breaking' this) and Stéphane (the author of
> this driver).
>
> Gents, any input on the above ?
>

It seems correct at a quick glance, but I'd like to investigate more. I
will do so when I'm back from vacation next week :)

Stéphane


> That said, please drop the unneeded brackets and add the following
> line in the commit message for v2.
> Cc: mesa-sta...@lists.freedesktop.org
>
> Thanks,
> Emil
> ___
> 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] i915g: fix incorrect gl_FragCoord value

[Emil Velikov]

Hi Nicholas,

On 26 August 2016 at 00:31, Nicholas Bishop  wrote:
> From: Nicholas Bishop 
>
> On Intel Pineview M hardware, the i915 gallium driver doesn't output
> the correct gl_FragCoord. It seems to always have an X coord of 0.0
> and a Y coord of the window's height in pixels, e.g. 600.0f or such.
>
> I believe this is a regression caused in part by this commit:
> afa035031ff9e0c07a2297d864e46c76f7bfff58
>
> The old behavior used the output at index zero, while the new behavior
> uses actual zeroes. In the case of gl_FragCoord the output at index
> zero happened to be the correct one, so the behavior appeared correct
> although the code already had a bug.
>
> Fixed by checking for I915_SEMANTIC_POS when setting up texCoords. If
> the generic_mapping is I915_SEMANTIC_POS, look for the
> TGSI_SEMANTIC_POSITION instead of a TGSI_SEMANTIC_GENERIC output.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=97477
> ---
>  src/gallium/drivers/i915/i915_state_derived.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/i915/i915_state_derived.c 
> b/src/gallium/drivers/i915/i915_state_derived.c
> index 177b854..dbfbc84 100644
> --- a/src/gallium/drivers/i915/i915_state_derived.c
> +++ b/src/gallium/drivers/i915/i915_state_derived.c
> @@ -145,7 +145,12 @@ static void calculate_vertex_layout(struct i915_context 
> *i915)
>uint hwtc;
>if (texCoords[i]) {
>   hwtc = TEXCOORDFMT_4D;
> - src = draw_find_shader_output(i915->draw, TGSI_SEMANTIC_GENERIC, 
> fs->generic_mapping[i]);
> + if (fs->generic_mapping[i] == I915_SEMANTIC_POS) {
> +src = draw_find_shader_output(i915->draw, 
> TGSI_SEMANTIC_POSITION, 0);
> + }
> + else {
> +src = draw_find_shader_output(i915->draw, TGSI_SEMANTIC_GENERIC, 
> fs->generic_mapping[i]);
> + }

Personally I'm unfamiliar with the i915g driver, so I've Cc'd Roland
(the author of the commit 'breaking' this) and Stéphane (the author of
this driver).

Gents, any input on the above ?

That said, please drop the unneeded brackets and add the following
line in the commit message for v2.
Cc: mesa-sta...@lists.freedesktop.org

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