| Message ID | 20240418074945.47349-2-umang.jain@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2024-04-18 08:49:44) > The Request pointer is cached in RkISP1FrameInfo. Use it directly > instead of retrieving it via buffer->request(). I don't think there's any specific speed up here. I think the compiler will map this into a single data read in both cases ? As this is in the bufferReady() context, I think useing the request associat3ed with the buffer is 'currently' correct. That said, I think this is an area that's likely about to get reworked in two fronts. Dan has looked at how to clean up the FrameInfo handling, and Stefan has been looking at how to be able to split/spearate buffers and requests. So I suspect that we should already merge 2/2 from this series but probably leave this one out for now... > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index abb21968..5a6c9e1e 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1261,7 +1261,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > return; > > const FrameMetadata &metadata = buffer->metadata(); > - Request *request = buffer->request(); > + Request *request = info->request; > > if (metadata.status != FrameMetadata::FrameCancelled) { > /* > -- > 2.44.0 >
Hi Umang, Thank you for the patch. On Thu, Apr 18, 2024 at 01:19:44PM +0530, Umang Jain wrote: > The Request pointer is cached in RkISP1FrameInfo. Use it directly > instead of retrieving it via buffer->request(). https://cbea.ms/git-commit/#why-not-how > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index abb21968..5a6c9e1e 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1261,7 +1261,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > return; > > const FrameMetadata &metadata = buffer->metadata(); > - Request *request = buffer->request(); > + Request *request = info->request; > > if (metadata.status != FrameMetadata::FrameCancelled) { > /*
Hi Kieran On 18/04/24 3:38 pm, Kieran Bingham wrote: > Quoting Umang Jain (2024-04-18 08:49:44) >> The Request pointer is cached in RkISP1FrameInfo. Use it directly >> instead of retrieving it via buffer->request(). > I don't think there's any specific speed up here. I think the compiler > will map this into a single data read in both cases ? > > As this is in the bufferReady() context, I think useing the request > associat3ed with the buffer is 'currently' correct. > > That said, I think this is an area that's likely about to get reworked > in two fronts. Dan has looked at how to clean up the FrameInfo handling, > and Stefan has been looking at how to be able to split/spearate buffers > and requests. There is a third front in this case, where the buffer is the internal buffer which pipes into another component. In that case, buffer->request() will fail (since buffer is an internal buffer in this case). Regarding the latter point, I thought it already is in line with separation of request/buffers ... since we are using cached value of request, instead of getting it from buffer > > So I suspect that we should already merge 2/2 from this series but > probably leave this one out for now... > OK > > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index abb21968..5a6c9e1e 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -1261,7 +1261,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) >> return; >> >> const FrameMetadata &metadata = buffer->metadata(); >> - Request *request = buffer->request(); >> + Request *request = info->request; >> >> if (metadata.status != FrameMetadata::FrameCancelled) { >> /* >> -- >> 2.44.0 >>
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index abb21968..5a6c9e1e 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1261,7 +1261,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) return; const FrameMetadata &metadata = buffer->metadata(); - Request *request = buffer->request(); + Request *request = info->request; if (metadata.status != FrameMetadata::FrameCancelled) { /*
The Request pointer is cached in RkISP1FrameInfo. Use it directly instead of retrieving it via buffer->request(). Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)