pipeline: simple: Increase buffer count to four

21392 diff mbox series

Which is not only what many other pipeline handlers use, but also a good
lower limit when dealing with DRM and similar APIs. Even Mesas EGL and
Vulkan WSI implementations use for the reason outlined in mesa commit
992a2dbba80aba35efe83202e1013bd6143f0dba:
> When the compositor is directly scanning out from the application's buffer it
> may end up holding on to three buffers. These are the one that is is currently
> scanning out from, one that has been given to DRM as the next buffer to flip
> to, and one that has been attached and will be given to DRM as soon as the
> previous flip completes. When we attach a fourth buffer to the compositor it
> should replace that third buffer so we should get a release event immediately
> after that. This patch therefore also changes the number of buffer slots to 4
> so that we can accomodate that situation.

Given the popularity of this buffer number the bump should be unlikely
to cause problems. At the same time it may help with performance or
even work around glitches.

The previous number was introduced in commit
a8964c28c80fb520ee3c7b10143371081d41405a without mentioning a specific
reason against the change at hand.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Robert,

Thank you for the patch.

On 27/09/24 2:37 am, Robert Mader wrote:
> Which is not only what many other pipeline handlers use, but also a good
> lower limit when dealing with DRM and similar APIs. Even Mesas EGL and
> Vulkan WSI implementations use for the reason outlined in mesa commit
> 992a2dbba80aba35efe83202e1013bd6143f0dba:
>> When the compositor is directly scanning out from the application's buffer it
>> may end up holding on to three buffers. These are the one that is is currently
>> scanning out from, one that has been given to DRM as the next buffer to flip
>> to, and one that has been attached and will be given to DRM as soon as the
>> previous flip completes. When we attach a fourth buffer to the compositor it
>> should replace that third buffer so we should get a release event immediately
>> after that. This patch therefore also changes the number of buffer slots to 4
>> so that we can accomodate that situation.
> Given the popularity of this buffer number the bump should be unlikely
> to cause problems. At the same time it may help with performance or
> even work around glitches.
>
> The previous number was introduced in commit
> a8964c28c80fb520ee3c7b10143371081d41405a without mentioning a specific
> reason against the change at hand.
>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>

Let's add to popularity then,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   src/libcamera/pipeline/simple/simple.cpp | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 81915573..78ce9232 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1135,7 +1135,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>   			cfg.frameSize = format.planes[0].size;
>   		}
>   
> -		cfg.bufferCount = 3;
> +		cfg.bufferCount = 4;
>   	}
>   
>   	return status;

Quoting Umang Jain (2024-09-27 08:09:48)
> Hi Robert,
> 
> Thank you for the patch.
> 
> On 27/09/24 2:37 am, Robert Mader wrote:
> > Which is not only what many other pipeline handlers use, but also a good
> > lower limit when dealing with DRM and similar APIs. Even Mesas EGL and
> > Vulkan WSI implementations use for the reason outlined in mesa commit
> > 992a2dbba80aba35efe83202e1013bd6143f0dba:
> >> When the compositor is directly scanning out from the application's buffer it
> >> may end up holding on to three buffers. These are the one that is is currently
> >> scanning out from, one that has been given to DRM as the next buffer to flip
> >> to, and one that has been attached and will be given to DRM as soon as the
> >> previous flip completes. When we attach a fourth buffer to the compositor it
> >> should replace that third buffer so we should get a release event immediately
> >> after that. This patch therefore also changes the number of buffer slots to 4
> >> so that we can accomodate that situation.
> > Given the popularity of this buffer number the bump should be unlikely
> > to cause problems. At the same time it may help with performance or
> > even work around glitches.
> >
> > The previous number was introduced in commit
> > a8964c28c80fb520ee3c7b10143371081d41405a without mentioning a specific
> > reason against the change at hand.
> >
> > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> 
> Let's add to popularity then,
> 
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

This is fine for me too.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> > ---
> >   src/libcamera/pipeline/simple/simple.cpp | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 81915573..78ce9232 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -1135,7 +1135,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >                       cfg.frameSize = format.planes[0].size;
> >               }
> >   
> > -             cfg.bufferCount = 3;
> > +             cfg.bufferCount = 4;
> >       }
> >   
> >       return status;
>