Re: [Mesa-dev] [PATCH] loader/dri3: Always use at least two back buffers

[Michel Dänzer]

On 06/09/16 03:14 PM, Michel Dänzer wrote:
> On 06/09/16 01:59 PM, Jason Ekstrand wrote:
>> On Sep 5, 2016 8:43 PM, "Michel Dänzer" > > wrote:
>>>
>>> On 06/09/16 12:08 PM, Jason Ekstrand wrote:
 On Mon, Sep 5, 2016 at 7:39 PM, Michel Dänzer > 
 >> wrote:

 From: Michel Dänzer > 
 >>

 This can make a significant difference for performance with some
>> extreme
 test cases such as vblank_mode=0 glxgears.

 Fixes: 1e3218bc5ba2 ("loader/dri3: Overhaul dri3_update_num_back")
 Cc: "12.0 11.2" > 
 > >>
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97549
 
 Signed-off-by: Michel Dänzer > 
 >>
 ---

 I could swear I tested vblank_mode=0 glxgears with my previous
>> change
 and couldn't measure any difference, but I can now, so I must have
 messed up my previous testing somehow... Apologies for any
>> inconvenience
 this caused.

  src/loader/loader_dri3_helper.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

 diff --git a/src/loader/loader_dri3_helper.c
 b/src/loader/loader_dri3_helper.c
 index 86ae5ae..3ce0352 100644
 --- a/src/loader/loader_dri3_helper.c
 +++ b/src/loader/loader_dri3_helper.c
 @@ -70,10 +70,8 @@ dri3_update_num_back(struct loader_dri3_drawable
 *draw)
  {
 if (draw->flipping)
draw->num_back = 3;
 -   else if (draw->vtable->get_swap_interval(draw) != 0)
 -  draw->num_back = 2;
 else
 -  draw->num_back = 1;
 +  draw->num_back = 2;


 With this change, the function is logically identical to the old
 function with the async flipping case removed.
>>>
>>> Not sure what you mean exactly, but I hope we can agree that the code is
>>> easier to understand and reason about now. :)
>>
>> I mean that you original patch made two functional changes: One was to
>> drop quadbuffering
> 
> As I explained, there was really no quad-buffering in the first place.
> 
> 
>> and the second was to single-buffer in the swapinterval==0 case.
>> This effectively reverts the second functional change while leaving
>> the first intact.
> 
> That's not the only remaining change either. E.g. before my changes,
> flipping with swap interval > 0 was using only two buffers, now it's
> three. (That was actually the main motivation for my first change)

To be precise, this change only applies when the X driver supports async
flips, otherwise it was already using three buffers before. (Note that
async flips are never used with swap interval > 0)


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] loader/dri3: Always use at least two back buffers

[Michel Dänzer]

On 06/09/16 01:59 PM, Jason Ekstrand wrote:
> On Sep 5, 2016 8:43 PM, "Michel Dänzer"  > wrote:
>>
>> On 06/09/16 12:08 PM, Jason Ekstrand wrote:
>> > On Mon, Sep 5, 2016 at 7:39 PM, Michel Dänzer  
>> > >> wrote:
>> >
>> > From: Michel Dänzer  
>> > >>
>> >
>> > This can make a significant difference for performance with some
> extreme
>> > test cases such as vblank_mode=0 glxgears.
>> >
>> > Fixes: 1e3218bc5ba2 ("loader/dri3: Overhaul dri3_update_num_back")
>> > Cc: "12.0 11.2"  
>> >  >>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97549
>> > 
>> > Signed-off-by: Michel Dänzer  
>> > >>
>> > ---
>> >
>> > I could swear I tested vblank_mode=0 glxgears with my previous
> change
>> > and couldn't measure any difference, but I can now, so I must have
>> > messed up my previous testing somehow... Apologies for any
> inconvenience
>> > this caused.
>> >
>> >  src/loader/loader_dri3_helper.c | 4 +---
>> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> > diff --git a/src/loader/loader_dri3_helper.c
>> > b/src/loader/loader_dri3_helper.c
>> > index 86ae5ae..3ce0352 100644
>> > --- a/src/loader/loader_dri3_helper.c
>> > +++ b/src/loader/loader_dri3_helper.c
>> > @@ -70,10 +70,8 @@ dri3_update_num_back(struct loader_dri3_drawable
>> > *draw)
>> >  {
>> > if (draw->flipping)
>> >draw->num_back = 3;
>> > -   else if (draw->vtable->get_swap_interval(draw) != 0)
>> > -  draw->num_back = 2;
>> > else
>> > -  draw->num_back = 1;
>> > +  draw->num_back = 2;
>> >
>> >
>> > With this change, the function is logically identical to the old
>> > function with the async flipping case removed.
>>
>> Not sure what you mean exactly, but I hope we can agree that the code is
>> easier to understand and reason about now. :)
> 
> I mean that you original patch made two functional changes: One was to
> drop quadbuffering

As I explained, there was really no quad-buffering in the first place.


> and the second was to single-buffer in the swapinterval==0 case.
> This effectively reverts the second functional change while leaving
> the first intact.

That's not the only remaining change either. E.g. before my changes,
flipping with swap interval > 0 was using only two buffers, now it's
three. (That was actually the main motivation for my first change)


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] loader/dri3: Always use at least two back buffers

[Jason Ekstrand]

On Sep 5, 2016 8:43 PM, "Michel Dänzer"  wrote:
>
> On 06/09/16 12:08 PM, Jason Ekstrand wrote:
> > On Mon, Sep 5, 2016 at 7:39 PM, Michel Dänzer  > > wrote:
> >
> > From: Michel Dänzer  > >
> >
> > This can make a significant difference for performance with some
extreme
> > test cases such as vblank_mode=0 glxgears.
> >
> > Fixes: 1e3218bc5ba2 ("loader/dri3: Overhaul dri3_update_num_back")
> > Cc: "12.0 11.2"  > >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97549
> > 
> > Signed-off-by: Michel Dänzer  > >
> > ---
> >
> > I could swear I tested vblank_mode=0 glxgears with my previous
change
> > and couldn't measure any difference, but I can now, so I must have
> > messed up my previous testing somehow... Apologies for any
inconvenience
> > this caused.
> >
> >  src/loader/loader_dri3_helper.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/src/loader/loader_dri3_helper.c
> > b/src/loader/loader_dri3_helper.c
> > index 86ae5ae..3ce0352 100644
> > --- a/src/loader/loader_dri3_helper.c
> > +++ b/src/loader/loader_dri3_helper.c
> > @@ -70,10 +70,8 @@ dri3_update_num_back(struct loader_dri3_drawable
> > *draw)
> >  {
> > if (draw->flipping)
> >draw->num_back = 3;
> > -   else if (draw->vtable->get_swap_interval(draw) != 0)
> > -  draw->num_back = 2;
> > else
> > -  draw->num_back = 1;
> > +  draw->num_back = 2;
> >
> >
> > With this change, the function is logically identical to the old
> > function with the async flipping case removed.
>
> Not sure what you mean exactly, but I hope we can agree that the code is
> easier to understand and reason about now. :)

I mean that you original patch made two functional changes: One was to drop
quadbuffering and the second was to single-buffer in the swapinterval==0
case.  This effectively reverts the second functional change while leaving
the first intact.

Yes, it's much easier to read now.  One could argue that the "counting
buffers" approach of the original code had it's merits, but it is much
easier to look at the new code and see what it will do in any particular
case.

> > I'm still not sure why we aren't getting a stall with 3 butters in the
> > swapinterval=0 flipping case, but if it doesn't have a perf impact, it
must
> > not be a problem.
>
> Right, there is a stall, but it doesn't seem significant, at least on my
> setup.
>
>
> > Reviewed-by: Jason Ekstrand  > >
>
> Thanks!
>
>
> --
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] loader/dri3: Always use at least two back buffers

[Michel Dänzer]

On 06/09/16 12:08 PM, Jason Ekstrand wrote:
> On Mon, Sep 5, 2016 at 7:39 PM, Michel Dänzer  > wrote:
> 
> From: Michel Dänzer  >
> 
> This can make a significant difference for performance with some extreme
> test cases such as vblank_mode=0 glxgears.
> 
> Fixes: 1e3218bc5ba2 ("loader/dri3: Overhaul dri3_update_num_back")
> Cc: "12.0 11.2"  >
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97549
> 
> Signed-off-by: Michel Dänzer  >
> ---
> 
> I could swear I tested vblank_mode=0 glxgears with my previous change
> and couldn't measure any difference, but I can now, so I must have
> messed up my previous testing somehow... Apologies for any inconvenience
> this caused.
> 
>  src/loader/loader_dri3_helper.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/loader/loader_dri3_helper.c
> b/src/loader/loader_dri3_helper.c
> index 86ae5ae..3ce0352 100644
> --- a/src/loader/loader_dri3_helper.c
> +++ b/src/loader/loader_dri3_helper.c
> @@ -70,10 +70,8 @@ dri3_update_num_back(struct loader_dri3_drawable
> *draw)
>  {
> if (draw->flipping)
>draw->num_back = 3;
> -   else if (draw->vtable->get_swap_interval(draw) != 0)
> -  draw->num_back = 2;
> else
> -  draw->num_back = 1;
> +  draw->num_back = 2;
> 
> 
> With this change, the function is logically identical to the old
> function with the async flipping case removed.

Not sure what you mean exactly, but I hope we can agree that the code is
easier to understand and reason about now. :)


> I'm still not sure why we aren't getting a stall with 3 butters in the
> swapinterval=0 flipping case, but if it doesn't have a perf impact, it must
> not be a problem.

Right, there is a stall, but it doesn't seem significant, at least on my
setup.


> Reviewed-by: Jason Ekstrand  >

Thanks!


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] loader/dri3: Always use at least two back buffers

[Jason Ekstrand]

On Mon, Sep 5, 2016 at 7:39 PM, Michel Dänzer  wrote:

> From: Michel Dänzer 
>
> This can make a significant difference for performance with some extreme
> test cases such as vblank_mode=0 glxgears.
>
> Fixes: 1e3218bc5ba2 ("loader/dri3: Overhaul dri3_update_num_back")
> Cc: "12.0 11.2" 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97549
> Signed-off-by: Michel Dänzer 
> ---
>
> I could swear I tested vblank_mode=0 glxgears with my previous change
> and couldn't measure any difference, but I can now, so I must have
> messed up my previous testing somehow... Apologies for any inconvenience
> this caused.
>
>  src/loader/loader_dri3_helper.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_
> helper.c
> index 86ae5ae..3ce0352 100644
> --- a/src/loader/loader_dri3_helper.c
> +++ b/src/loader/loader_dri3_helper.c
> @@ -70,10 +70,8 @@ dri3_update_num_back(struct loader_dri3_drawable *draw)
>  {
> if (draw->flipping)
>draw->num_back = 3;
> -   else if (draw->vtable->get_swap_interval(draw) != 0)
> -  draw->num_back = 2;
> else
> -  draw->num_back = 1;
> +  draw->num_back = 2;
>

With this change, the function is logically identical to the old function
with the async flipping case removed.  I'm still not sure why we aren't
getting a stall with 3 butters in the swapinterval=0 flipping case, but if
it doesn't have a perf impact, it must not be a problem.

Reviewed-by: Jason Ekstrand 

Thanks for digging into it to find the real problem. :-)

--Jason


>  }
>
>  void
> --
> 2.9.3
>
> ___
> 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