| Message ID | 20211110100802.349623-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck (2021-11-10 10:08:02) > If a stream is marked as external, double the number of V4L2BufferCache slots > that are allocated. This is to account for additional buffers that may be > allocated directly by the application. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > index b3265d0e8aab..67901936d6b6 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) > count = bufferMap_.size(); > } > > + /* > + * If this is an external stream, we must allocate slots for buffers that > + * might be externally allocated. We have no indication of how many buffers > + * may be used, so this might overallocate slots in the buffer cache. > + */ Perhaps we'll need a better system here (not in this patch) to handle this ... but I think overallocating is cheap - and the whole point of the buffer cache is to try to re-use the same buffers where possible. So I /think/ overallocations is the right solution for now anyway. > + if (isExternal()) > + count = count * 2; Of course it's hard to know /how/ far to overallocate ... But this will do until we figure it out ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > return dev_->importBuffers(count); > } > > -- > 2.25.1 >
Hi Kieran, Thanks for all the reviews! On Wed, 10 Nov 2021 at 10:33, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2021-11-10 10:08:02) > > If a stream is marked as external, double the number of V4L2BufferCache > slots > > that are allocated. This is to account for additional buffers that may be > > allocated directly by the application. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > index b3265d0e8aab..67901936d6b6 100644 > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) > > count = bufferMap_.size(); > > } > > > > + /* > > + * If this is an external stream, we must allocate slots for > buffers that > > + * might be externally allocated. We have no indication of how > many buffers > > + * may be used, so this might overallocate slots in the buffer > cache. > > + */ > > Perhaps we'll need a better system here (not in this patch) to handle > this ... but I think overallocating is cheap - and the whole point of > the buffer cache is to try to re-use the same buffers where possible. So > I /think/ overallocations is the right solution for now anyway. > I think this over allocation is the only change needed to "fix" Roman's original issue. But I did mean to tidy up the buffer allocations for some time now, so why not :-) > > > + if (isExternal()) > > + count = count * 2; > > Of course it's hard to know /how/ far to overallocate ... But this will > do until we figure it out ;-) > I wonder if we can perhaps allow the cache to grow in size dynamically instead of relying on a fixed number of slots? Naush > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > return dev_->importBuffers(count); > > } > > > > -- > > 2.25.1 > > >
Quoting Naushir Patuck (2021-11-10 10:42:58) > Hi Kieran, > > Thanks for all the reviews! > > On Wed, 10 Nov 2021 at 10:33, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Quoting Naushir Patuck (2021-11-10 10:08:02) > > > If a stream is marked as external, double the number of V4L2BufferCache > > slots > > > that are allocated. This is to account for additional buffers that may be > > > allocated directly by the application. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > index b3265d0e8aab..67901936d6b6 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) > > > count = bufferMap_.size(); > > > } > > > > > > + /* > > > + * If this is an external stream, we must allocate slots for > > buffers that > > > + * might be externally allocated. We have no indication of how > > many buffers > > > + * may be used, so this might overallocate slots in the buffer > > cache. > > > + */ > > > > Perhaps we'll need a better system here (not in this patch) to handle > > this ... but I think overallocating is cheap - and the whole point of > > the buffer cache is to try to re-use the same buffers where possible. So > > I /think/ overallocations is the right solution for now anyway. > > > > I think this over allocation is the only change needed to "fix" Roman's > original > issue. But I did mean to tidy up the buffer allocations for some time now, > so > why not :-) > > > > > > > + if (isExternal()) > > > + count = count * 2; > > > > Of course it's hard to know /how/ far to overallocate ... But this will > > do until we figure it out ;-) > > > > I wonder if we can perhaps allow the cache to grow in size dynamically > instead of relying on a fixed number of slots? That's the bit that I wondered about as the "better system" - but it would require a matching number of 'buffers' on the v4l2 side - so dynamically growing the slots would have to interact with the V4L2 buffer allcoations I think... I believe we can ask for more buffers some how - but I don't know if we can do so /while/ streaming... -- Kieran > Naush > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > + > > > return dev_->importBuffers(count); > > > } > > > > > > -- > > > 2.25.1 > > > > >
Hi Naush Thank you for the patch On 11/10/21 3:38 PM, Naushir Patuck wrote: > If a stream is marked as external, double the number of V4L2BufferCache slots > that are allocated. This is to account for additional buffers that may be > allocated directly by the application. One clarification pleease, does this mean applications can still allocate more buffers on the fly (i.e. the count can increase in future), even after RPi pipeline-handler has started? > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Patch looks good so, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > index b3265d0e8aab..67901936d6b6 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) > count = bufferMap_.size(); > } > > + /* > + * If this is an external stream, we must allocate slots for buffers that > + * might be externally allocated. We have no indication of how many buffers > + * may be used, so this might overallocate slots in the buffer cache. > + */ > + if (isExternal()) > + count = count * 2; > + > return dev_->importBuffers(count); > } >
Hi Umang, Thank you for your reviews! On Wed, 10 Nov 2021 at 18:35, Umang Jain <umang.jain@ideasonboard.com> wrote: > Hi Naush > > Thank you for the patch > > On 11/10/21 3:38 PM, Naushir Patuck wrote: > > If a stream is marked as external, double the number of V4L2BufferCache > slots > > that are allocated. This is to account for additional buffers that may be > > allocated directly by the application. > > > One clarification pleease, does this mean applications can still > allocate more buffers on the fly (i.e. the count can increase in > future), even after RPi pipeline-handler has started? > My understanding is that the Android layer does exactly this. As pointed out by Kieran, one issue is that we may not know the exact number of buffers allocated by the application. Hence we need a mechanism where the buffer cache sizing might have to become dynamic to account for additional buffers. For now, over allocating the slots in the cache will be sufficient. Regards, Naush > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Patch looks good so, > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > index b3265d0e8aab..67901936d6b6 100644 > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) > > count = bufferMap_.size(); > > } > > > > + /* > > + * If this is an external stream, we must allocate slots for > buffers that > > + * might be externally allocated. We have no indication of how > many buffers > > + * may be used, so this might overallocate slots in the buffer > cache. > > + */ > > + if (isExternal()) > > + count = count * 2; > > + > > return dev_->importBuffers(count); > > } > > >
Naush,
Please let me once again remind you that I have to set the
configuration variable by downstream patch which determines the max.
number of buffers for Android to allocate.
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -429,9 +429,9 @@ CameraConfiguration::Status
RPiCameraConfiguration::validate()
cfg.frameSize = unicamFormat.planes[0].size;
rawCount++;
} else {
+ cfg.bufferCount = 3;
outSize[outCount] = std::make_pair(count, cfg.size);
/* Record the largest resolution for fixups later. */
if (maxSize < cfg.size) {
So maybe this value could be used to calculate cache size. Sorry if I
am talking about wrong things, I am new to the libcamera codebase.
Roman.
чт, 11 нояб. 2021 г. в 10:16, Naushir Patuck <naush@raspberrypi.com>:
>
> Hi Umang,
>
> Thank you for your reviews!
>
> On Wed, 10 Nov 2021 at 18:35, Umang Jain <umang.jain@ideasonboard.com> wrote:
>>
>> Hi Naush
>>
>> Thank you for the patch
>>
>> On 11/10/21 3:38 PM, Naushir Patuck wrote:
>> > If a stream is marked as external, double the number of V4L2BufferCache slots
>> > that are allocated. This is to account for additional buffers that may be
>> > allocated directly by the application.
>>
>>
>> One clarification pleease, does this mean applications can still
>> allocate more buffers on the fly (i.e. the count can increase in
>> future), even after RPi pipeline-handler has started?
>
>
> My understanding is that the Android layer does exactly this.
>
> As pointed out by Kieran, one issue is that we may not know
> the exact number of buffers allocated by the application. Hence
> we need a mechanism where the buffer cache sizing might have
> to become dynamic to account for additional buffers. For now,
> over allocating the slots in the cache will be sufficient.
>
> Regards,
> Naush
>
>>
>>
>> >
>> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>
>> Patch looks good so,
>>
>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>>
>> > ---
>> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++
>> > 1 file changed, 8 insertions(+)
>> >
>> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>> > index b3265d0e8aab..67901936d6b6 100644
>> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>> > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count)
>> > count = bufferMap_.size();
>> > }
>> >
>> > + /*
>> > + * If this is an external stream, we must allocate slots for buffers that
>> > + * might be externally allocated. We have no indication of how many buffers
>> > + * may be used, so this might overallocate slots in the buffer cache.
>> > + */
>> > + if (isExternal())
>> > + count = count * 2;
>> > +
>> > return dev_->importBuffers(count);
>> > }
>> >
Hi Roman, On Thu, 11 Nov 2021 at 08:36, Roman Stratiienko <r.stratiienko@gmail.com> wrote: > Naush, > > Please let me once again remind you that I have to set the > configuration variable by downstream patch which determines the max. > number of buffers for Android to allocate. > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -429,9 +429,9 @@ CameraConfiguration::Status > RPiCameraConfiguration::validate() > cfg.frameSize = unicamFormat.planes[0].size; > rawCount++; > } else { > + cfg.bufferCount = 3; > outSize[outCount] = std::make_pair(count, > cfg.size); > /* Record the largest resolution for fixups later. > */ > if (maxSize < cfg.size) { > > So maybe this value could be used to calculate cache size. Sorry if I > am talking about wrong things, I am new to the libcamera codebase. > I think this change effectively over allocates the buffer cache, and fixes the original problem you were seeing. The buffercount does indirectly get used to size the buffer cache in the existing code. I would not necessarily say this is the correct fix though, and we probably need to understand the Android layer's buffer allocation mechanism better for the Raspberry Pi pipeline handler. Naush > > Roman. > > чт, 11 нояб. 2021 г. в 10:16, Naushir Patuck <naush@raspberrypi.com>: > > > > Hi Umang, > > > > Thank you for your reviews! > > > > On Wed, 10 Nov 2021 at 18:35, Umang Jain <umang.jain@ideasonboard.com> > wrote: > >> > >> Hi Naush > >> > >> Thank you for the patch > >> > >> On 11/10/21 3:38 PM, Naushir Patuck wrote: > >> > If a stream is marked as external, double the number of > V4L2BufferCache slots > >> > that are allocated. This is to account for additional buffers that > may be > >> > allocated directly by the application. > >> > >> > >> One clarification pleease, does this mean applications can still > >> allocate more buffers on the fly (i.e. the count can increase in > >> future), even after RPi pipeline-handler has started? > > > > > > My understanding is that the Android layer does exactly this. > > > > As pointed out by Kieran, one issue is that we may not know > > the exact number of buffers allocated by the application. Hence > > we need a mechanism where the buffer cache sizing might have > > to become dynamic to account for additional buffers. For now, > > over allocating the slots in the cache will be sufficient. > > > > Regards, > > Naush > > > >> > >> > >> > > >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >> > >> Patch looks good so, > >> > >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >> > >> > --- > >> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ > >> > 1 file changed, 8 insertions(+) > >> > > >> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > >> > index b3265d0e8aab..67901936d6b6 100644 > >> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > >> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > >> > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) > >> > count = bufferMap_.size(); > >> > } > >> > > >> > + /* > >> > + * If this is an external stream, we must allocate slots for > buffers that > >> > + * might be externally allocated. We have no indication of how > many buffers > >> > + * may be used, so this might overallocate slots in the buffer > cache. > >> > + */ > >> > + if (isExternal()) > >> > + count = count * 2; > >> > + > >> > return dev_->importBuffers(count); > >> > } > >> > >
On Wed, Nov 10, 2021 at 10:52:02AM +0000, Kieran Bingham wrote: > Quoting Naushir Patuck (2021-11-10 10:42:58) > > On Wed, 10 Nov 2021 at 10:33, Kieran Bingham wrote: > > > Quoting Naushir Patuck (2021-11-10 10:08:02) > > > > If a stream is marked as external, double the number of V4L2BufferCache slots > > > > that are allocated. This is to account for additional buffers that may be > > > > allocated directly by the application. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > > index b3265d0e8aab..67901936d6b6 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > > @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) > > > > count = bufferMap_.size(); > > > > } > > > > > > > > + /* > > > > + * If this is an external stream, we must allocate slots for buffers that > > > > + * might be externally allocated. We have no indication of how many buffers > > > > + * may be used, so this might overallocate slots in the buffer cache. > > > > + */ > > > > > > Perhaps we'll need a better system here (not in this patch) to handle > > > this ... but I think overallocating is cheap - and the whole point of > > > the buffer cache is to try to re-use the same buffers where possible. So > > > I /think/ overallocations is the right solution for now anyway. Overallocation of V4L2 buffer slots is cheap, but we indeed can't know in advance how much to overallocate. Note that there's a hardcoded limit of 32 buffers in the kernel (VIDEO_MAX_FRAME). > > I think this over allocation is the only change needed to "fix" Roman's original > > issue. But I did mean to tidy up the buffer allocations for some time now, so > > why not :-) > > > > > > + if (isExternal()) > > > > + count = count * 2; > > > > > > Of course it's hard to know /how/ far to overallocate ... But this will > > > do until we figure it out ;-) > > > > I wonder if we can perhaps allow the cache to grow in size dynamically > > instead of relying on a fixed number of slots? > > That's the bit that I wondered about as the "better system" - but it > would require a matching number of 'buffers' on the v4l2 side - so > dynamically growing the slots would have to interact with the V4L2 > buffer allcoations I think... > > I believe we can ask for more buffers some how - but I don't know if we > can do so /while/ streaming... There's VIDIOC_CREATE_BUFS for that. The question is when to create new buffer slots, as if applications were to create new buffers all the time, the cache would keep growing. We probably need some type of aging system to decide when a previously cached entry would be old enough to be reused. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > + > > > > return dev_->importBuffers(count); > > > > } > > > >
diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp index b3265d0e8aab..67901936d6b6 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp @@ -107,6 +107,14 @@ int Stream::prepareBuffers(unsigned int count) count = bufferMap_.size(); } + /* + * If this is an external stream, we must allocate slots for buffers that + * might be externally allocated. We have no indication of how many buffers + * may be used, so this might overallocate slots in the buffer cache. + */ + if (isExternal()) + count = count * 2; + return dev_->importBuffers(count); }
If a stream is marked as external, double the number of V4L2BufferCache slots that are allocated. This is to account for additional buffers that may be allocated directly by the application. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)