| Message ID | 20250808040453.9016-1-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2025-08-08 05:04:53) > Explicitly clarify the usage of Request::ReuseBuffers flag in context > of buffer fences. Fences are user-supplied and are not re-cycled as > part of Request::reuse(), hence document this behaviour explicitly. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > src/libcamera/request.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 7f1e11e8..86d849ac 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -326,7 +326,8 @@ void Request::Private::timeout() > * \var Request::Default > * Don't reuse buffers > * \var Request::ReuseBuffers > - * Reuse the buffers that were previously added by addBuffer() > + * Reuse the buffers that were previously added by addBuffer(). > + * Buffers meant to be queued with a Fence, should not use this flag. Can we codify this in some way during Request::reuse() and check at runtime too to print a warning? I would bet if someone hits this they haven't read the documentation. -- Kieran > */ > > /** > -- > 2.50.0 >
On 2025-08-08 15:17, Kieran Bingham wrote: > Quoting Umang Jain (2025-08-08 05:04:53) >> Explicitly clarify the usage of Request::ReuseBuffers flag in context >> of buffer fences. Fences are user-supplied and are not re-cycled as >> part of Request::reuse(), hence document this behaviour explicitly. >> >> Signed-off-by: Umang Jain <uajain@igalia.com> >> --- >> src/libcamera/request.cpp | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >> index 7f1e11e8..86d849ac 100644 >> --- a/src/libcamera/request.cpp >> +++ b/src/libcamera/request.cpp >> @@ -326,7 +326,8 @@ void Request::Private::timeout() >> * \var Request::Default >> * Don't reuse buffers >> * \var Request::ReuseBuffers >> - * Reuse the buffers that were previously added by addBuffer() >> + * Reuse the buffers that were previously added by addBuffer(). >> + * Buffers meant to be queued with a Fence, should not use this flag. > > Can we codify this in some way during Request::reuse() and check at > runtime too to print a warning? I would bet if someone hits this they > haven't read the documentation. We can codify yes, but it won't be as straight-forward. We will need to track that the buffer had a fence set previously via a bool member and a getter for the same, in the FrameBuffer::Private class. The existing fence() won't work I believe, as the fence could have been released or signalled and fence_ will become nullptr. A second option would be to track at Request level and set a similar bool class member in Request::addBuffer() if the fence was passed in. This would fit better I think, if we decide to go to codify it. > > -- > Kieran > >> */ >> >> /** >> -- >> 2.50.0 >>
Quoting uajain (2025-08-08 11:47:54) > On 2025-08-08 15:17, Kieran Bingham wrote: > > Quoting Umang Jain (2025-08-08 05:04:53) > >> Explicitly clarify the usage of Request::ReuseBuffers flag in context > >> of buffer fences. Fences are user-supplied and are not re-cycled as > >> part of Request::reuse(), hence document this behaviour explicitly. > >> > >> Signed-off-by: Umang Jain <uajain@igalia.com> > >> --- > >> src/libcamera/request.cpp | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > >> index 7f1e11e8..86d849ac 100644 > >> --- a/src/libcamera/request.cpp > >> +++ b/src/libcamera/request.cpp > >> @@ -326,7 +326,8 @@ void Request::Private::timeout() > >> * \var Request::Default > >> * Don't reuse buffers > >> * \var Request::ReuseBuffers > >> - * Reuse the buffers that were previously added by addBuffer() > >> + * Reuse the buffers that were previously added by addBuffer(). > >> + * Buffers meant to be queued with a Fence, should not use this flag. > > > > Can we codify this in some way during Request::reuse() and check at > > runtime too to print a warning? I would bet if someone hits this they > > haven't read the documentation. > > We can codify yes, but it won't be as straight-forward. We will need to > track > that the buffer had a fence set previously via a bool member and a > getter for the same, > in the FrameBuffer::Private class. The existing fence() won't work I > believe, as the fence > could have been released or signalled and fence_ will become nullptr. Ah - I was hoping it was going to be a simple flag check on the existing fence_ or such - but if it gets reset when the fence is consumed indeed we won't know - and would have to store extra state which would possibly be undesirable... > A second option would be to track at Request level and set a similar > bool class member > in Request::addBuffer() if the fence was passed in. This would fit > better I think, > if we decide to go to codify it. Yes, if this is a path - knowing if a Request has 'needed' fences would be the signal to not let reuse be automatic. -- Kieran > > > > > -- > > Kieran > > > >> */ > >> > >> /** > >> -- > >> 2.50.0 > >>
Quoting Kieran Bingham (2025-08-08 11:56:47) > Quoting uajain (2025-08-08 11:47:54) > > On 2025-08-08 15:17, Kieran Bingham wrote: > > > Quoting Umang Jain (2025-08-08 05:04:53) > > >> Explicitly clarify the usage of Request::ReuseBuffers flag in context > > >> of buffer fences. Fences are user-supplied and are not re-cycled as > > >> part of Request::reuse(), hence document this behaviour explicitly. > > >> > > >> Signed-off-by: Umang Jain <uajain@igalia.com> > > >> --- > > >> src/libcamera/request.cpp | 3 ++- > > >> 1 file changed, 2 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > >> index 7f1e11e8..86d849ac 100644 > > >> --- a/src/libcamera/request.cpp > > >> +++ b/src/libcamera/request.cpp > > >> @@ -326,7 +326,8 @@ void Request::Private::timeout() > > >> * \var Request::Default > > >> * Don't reuse buffers > > >> * \var Request::ReuseBuffers > > >> - * Reuse the buffers that were previously added by addBuffer() > > >> + * Reuse the buffers that were previously added by addBuffer(). > > >> + * Buffers meant to be queued with a Fence, should not use this flag. > > > > > > Can we codify this in some way during Request::reuse() and check at > > > runtime too to print a warning? I would bet if someone hits this they > > > haven't read the documentation. > > > > We can codify yes, but it won't be as straight-forward. We will need to > > track > > that the buffer had a fence set previously via a bool member and a > > getter for the same, > > in the FrameBuffer::Private class. The existing fence() won't work I > > believe, as the fence > > could have been released or signalled and fence_ will become nullptr. > > Ah - I was hoping it was going to be a simple flag check on the existing > fence_ or such - but if it gets reset when the fence is consumed indeed > we won't know - and would have to store extra state which would possibly > be undesirable... > > > A second option would be to track at Request level and set a similar > > bool class member > > in Request::addBuffer() if the fence was passed in. This would fit > > better I think, > > if we decide to go to codify it. > > Yes, if this is a path - knowing if a Request has 'needed' fences would > be the signal to not let reuse be automatic. > Anyway, I think the clarification itself here still makes sense so far - but Jacopo's the expert on Fences I think... and he just went on holiday... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > -- > Kieran > > > > > > > > > -- > > > Kieran > > > > > >> */ > > >> > > >> /** > > >> -- > > >> 2.50.0 > > >>
On 2025-08-08 16:27, Kieran Bingham wrote: > Quoting Kieran Bingham (2025-08-08 11:56:47) >> Quoting uajain (2025-08-08 11:47:54) >> > On 2025-08-08 15:17, Kieran Bingham wrote: >> > > Quoting Umang Jain (2025-08-08 05:04:53) >> > >> Explicitly clarify the usage of Request::ReuseBuffers flag in context >> > >> of buffer fences. Fences are user-supplied and are not re-cycled as >> > >> part of Request::reuse(), hence document this behaviour explicitly. >> > >> >> > >> Signed-off-by: Umang Jain <uajain@igalia.com> >> > >> --- >> > >> src/libcamera/request.cpp | 3 ++- >> > >> 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> >> > >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >> > >> index 7f1e11e8..86d849ac 100644 >> > >> --- a/src/libcamera/request.cpp >> > >> +++ b/src/libcamera/request.cpp >> > >> @@ -326,7 +326,8 @@ void Request::Private::timeout() >> > >> * \var Request::Default >> > >> * Don't reuse buffers >> > >> * \var Request::ReuseBuffers >> > >> - * Reuse the buffers that were previously added by addBuffer() >> > >> + * Reuse the buffers that were previously added by addBuffer(). >> > >> + * Buffers meant to be queued with a Fence, should not use this flag. >> > > >> > > Can we codify this in some way during Request::reuse() and check at >> > > runtime too to print a warning? I would bet if someone hits this they >> > > haven't read the documentation. >> > >> > We can codify yes, but it won't be as straight-forward. We will need to >> > track >> > that the buffer had a fence set previously via a bool member and a >> > getter for the same, >> > in the FrameBuffer::Private class. The existing fence() won't work I >> > believe, as the fence >> > could have been released or signalled and fence_ will become nullptr. >> >> Ah - I was hoping it was going to be a simple flag check on the existing >> fence_ or such - but if it gets reset when the fence is consumed indeed >> we won't know - and would have to store extra state which would possibly >> be undesirable... >> >> > A second option would be to track at Request level and set a similar >> > bool class member >> > in Request::addBuffer() if the fence was passed in. This would fit >> > better I think, >> > if we decide to go to codify it. >> >> Yes, if this is a path - knowing if a Request has 'needed' fences would >> be the signal to not let reuse be automatic. >> > > Anyway, I think the clarification itself here still makes sense so far - > but Jacopo's the expert on Fences I think... Yes, I wanted to check first if it is worth tracking the extra 'state' and checking it, specially when this can be a intended hot-path. > > and he just went on holiday... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> -- >> Kieran >> >> > >> > > >> > > -- >> > > Kieran >> > > >> > >> */ >> > >> >> > >> /** >> > >> -- >> > >> 2.50.0 >> > >>
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 7f1e11e8..86d849ac 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -326,7 +326,8 @@ void Request::Private::timeout() * \var Request::Default * Don't reuse buffers * \var Request::ReuseBuffers - * Reuse the buffers that were previously added by addBuffer() + * Reuse the buffers that were previously added by addBuffer(). + * Buffers meant to be queued with a Fence, should not use this flag. */ /**
Explicitly clarify the usage of Request::ReuseBuffers flag in context of buffer fences. Fences are user-supplied and are not re-cycled as part of Request::reuse(), hence document this behaviour explicitly. Signed-off-by: Umang Jain <uajain@igalia.com> --- src/libcamera/request.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)