| Message ID | 20200720224232.153717-8-kieran.bingham@ideasonboard.com |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, (CC'ing Tomasz to get a review from a Chrome OS point of view) Thank you for the patch. On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote: > Use lseek to query the length of planes where possible rather than leaving > the plane.length as zero, which prevents mapping buffers for software > processing. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > I like this one ;-) We need to know the plane lengths for software > streams now, but the correct way to get this would be to talk to the > gralloc allocator. This would mean pulling in various extra > cros-specific libraries, where instead we can query the size of the > dmabuf by getting the offset when we SEEK_END. > > > src/android/camera_device.cpp | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 538b8ab5da03..6212ccdd61ec 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer > continue; > } > > - /* > - * Setting length to zero here is OK as the length is only used > - * to map the memory of the plane. Libcamera do not need to poke > - * at the memory content queued by the HAL. > - */ > - plane.length = 0; > + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > + if (length == -1) { > + /* > + * A zero length plane is not fatal unless the > + * FrameBuffer is used for a software stream, libcamera > + * itself will not access the internal frame content. > + */ > + LOG(HAL, Error) << "Failed to query plane length"; > + length = 0; Do you expect this to happen ? If not, I'd return an error. If yes, an error message would be printed in a loop, which isn't very nice. My preference would be to return an error if possible. Otherwise, nice hack :-) > + } > + > + plane.length = length; > planes.push_back(std::move(plane)); > } >
On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kieran, > > (CC'ing Tomasz to get a review from a Chrome OS point of view) > > Thank you for the patch. > > On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote: > > Use lseek to query the length of planes where possible rather than leaving > > the plane.length as zero, which prevents mapping buffers for software > > processing. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > > > I like this one ;-) We need to know the plane lengths for software > > streams now, but the correct way to get this would be to talk to the > > gralloc allocator. This would mean pulling in various extra > > cros-specific libraries, where instead we can query the size of the > > dmabuf by getting the offset when we SEEK_END. > > > > > > src/android/camera_device.cpp | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 538b8ab5da03..6212ccdd61ec 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer > > continue; > > } > > > > - /* > > - * Setting length to zero here is OK as the length is only used > > - * to map the memory of the plane. Libcamera do not need to poke > > - * at the memory content queued by the HAL. > > - */ > > - plane.length = 0; > > + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > > + if (length == -1) { > > + /* > > + * A zero length plane is not fatal unless the > > + * FrameBuffer is used for a software stream, libcamera > > + * itself will not access the internal frame content. > > + */ > > + LOG(HAL, Error) << "Failed to query plane length"; > > + length = 0; > > Do you expect this to happen ? If not, I'd return an error. If yes, an > error message would be printed in a loop, which isn't very nice. My > preference would be to return an error if possible. > > Otherwise, nice hack :-) A zero-length plane would be strange and could suggest that the backing file is not a DMA-buf, so it could indeed make sense to error out. Otherwise, we tend to do this in a number of places in Chrome OS and seems to be the official way to query a DMA-buf for its size. However I wonder if we shouldn't restore the original position after the seek? Best regards, Tomasz
Hi All, On 27/07/2020 14:45, Tomasz Figa wrote: > On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hi Kieran, >> >> (CC'ing Tomasz to get a review from a Chrome OS point of view) >> >> Thank you for the patch. >> >> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote: >>> Use lseek to query the length of planes where possible rather than leaving >>> the plane.length as zero, which prevents mapping buffers for software >>> processing. >>> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> >>> I like this one ;-) We need to know the plane lengths for software >>> streams now, but the correct way to get this would be to talk to the >>> gralloc allocator. This would mean pulling in various extra >>> cros-specific libraries, where instead we can query the size of the >>> dmabuf by getting the offset when we SEEK_END. >>> >>> >>> src/android/camera_device.cpp | 18 ++++++++++++------ >>> 1 file changed, 12 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index 538b8ab5da03..6212ccdd61ec 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer >>> continue; >>> } >>> >>> - /* >>> - * Setting length to zero here is OK as the length is only used >>> - * to map the memory of the plane. Libcamera do not need to poke >>> - * at the memory content queued by the HAL. >>> - */ >>> - plane.length = 0; >>> + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); >>> + if (length == -1) { >>> + /* >>> + * A zero length plane is not fatal unless the >>> + * FrameBuffer is used for a software stream, libcamera >>> + * itself will not access the internal frame content. >>> + */ >>> + LOG(HAL, Error) << "Failed to query plane length"; >>> + length = 0; >> >> Do you expect this to happen ? If not, I'd return an error. If yes, an >> error message would be printed in a loop, which isn't very nice. My >> preference would be to return an error if possible. >> I don't think I expect it to happen. >> Otherwise, nice hack :-) > > A zero-length plane would be strange and could suggest that the > backing file is not a DMA-buf, so it could indeed make sense to error > out. I suspect I coded this thinking that 0 was currently acceptable, so it might still be acceptable to use. But I agree with you both that if we can't determine the length of the buffer then we have an error and should highlight it accordingly. > Otherwise, we tend to do this in a number of places in Chrome OS and > seems to be the official way to query a DMA-buf for its size. However > I wonder if we shouldn't restore the original position after the seek? Does CrOS do this? It's an extra couple of calls, (SEEK_CUR, and SEEK_SET), but I hope they're not too expensive ... I would expect anyone using a DMABuf will mmap the data if needed rather than use read()/write(), but that's probably making too big an assumption, so indeed if we left it at SEEK_END we might break something ... -- Kieran > Best regards, > Tomasz >
On Mon, Jul 27, 2020 at 4:21 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi All, > > On 27/07/2020 14:45, Tomasz Figa wrote: > > On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > >> > >> Hi Kieran, > >> > >> (CC'ing Tomasz to get a review from a Chrome OS point of view) > >> > >> Thank you for the patch. > >> > >> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote: > >>> Use lseek to query the length of planes where possible rather than leaving > >>> the plane.length as zero, which prevents mapping buffers for software > >>> processing. > >>> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> > >>> I like this one ;-) We need to know the plane lengths for software > >>> streams now, but the correct way to get this would be to talk to the > >>> gralloc allocator. This would mean pulling in various extra > >>> cros-specific libraries, where instead we can query the size of the > >>> dmabuf by getting the offset when we SEEK_END. > >>> > >>> > >>> src/android/camera_device.cpp | 18 ++++++++++++------ > >>> 1 file changed, 12 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>> index 538b8ab5da03..6212ccdd61ec 100644 > >>> --- a/src/android/camera_device.cpp > >>> +++ b/src/android/camera_device.cpp > >>> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer > >>> continue; > >>> } > >>> > >>> - /* > >>> - * Setting length to zero here is OK as the length is only used > >>> - * to map the memory of the plane. Libcamera do not need to poke > >>> - * at the memory content queued by the HAL. > >>> - */ > >>> - plane.length = 0; > >>> + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > >>> + if (length == -1) { > >>> + /* > >>> + * A zero length plane is not fatal unless the > >>> + * FrameBuffer is used for a software stream, libcamera > >>> + * itself will not access the internal frame content. > >>> + */ > >>> + LOG(HAL, Error) << "Failed to query plane length"; > >>> + length = 0; > >> > >> Do you expect this to happen ? If not, I'd return an error. If yes, an > >> error message would be printed in a loop, which isn't very nice. My > >> preference would be to return an error if possible. > >> > > I don't think I expect it to happen. > > >> Otherwise, nice hack :-) > > > > A zero-length plane would be strange and could suggest that the > > backing file is not a DMA-buf, so it could indeed make sense to error > > out. > > I suspect I coded this thinking that 0 was currently acceptable, so it > might still be acceptable to use. > > But I agree with you both that if we can't determine the length of the > buffer then we have an error and should highlight it accordingly. > > > > Otherwise, we tend to do this in a number of places in Chrome OS and > > seems to be the official way to query a DMA-buf for its size. However > > I wonder if we shouldn't restore the original position after the seek? > > Does CrOS do this? It's an extra couple of calls, (SEEK_CUR, and > SEEK_SET), but I hope they're not too expensive ... > > > > I would expect anyone using a DMABuf will mmap the data if needed rather > than use read()/write(), but that's probably making too big an > assumption, so indeed if we left it at SEEK_END we might break something ... The views on this are separated even in Chrome OS. There are places where we simply set it back to 0 and there are places where we do a proper save, change and restore. I don't have a strong opinion on this and I'm pretty much sure we don't have any read/write calls to DMA-bufs in Chrome OS, but there could be some implications on other platforms, so I've intentionally made the last sentence a question. :) Best regards, Tomasz
Hi Tomasz, On Mon, Jul 27, 2020 at 04:30:08PM +0200, Tomasz Figa wrote: > On Mon, Jul 27, 2020 at 4:21 PM Kieran Bingham wrote: > > On 27/07/2020 14:45, Tomasz Figa wrote: > >> On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart wrote: > >>> > >>> Hi Kieran, > >>> > >>> (CC'ing Tomasz to get a review from a Chrome OS point of view) > >>> > >>> Thank you for the patch. > >>> > >>> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote: > >>>> Use lseek to query the length of planes where possible rather than leaving > >>>> the plane.length as zero, which prevents mapping buffers for software > >>>> processing. > >>>> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> --- > >>>> > >>>> I like this one ;-) We need to know the plane lengths for software > >>>> streams now, but the correct way to get this would be to talk to the > >>>> gralloc allocator. This would mean pulling in various extra > >>>> cros-specific libraries, where instead we can query the size of the > >>>> dmabuf by getting the offset when we SEEK_END. > >>>> > >>>> > >>>> src/android/camera_device.cpp | 18 ++++++++++++------ > >>>> 1 file changed, 12 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>>> index 538b8ab5da03..6212ccdd61ec 100644 > >>>> --- a/src/android/camera_device.cpp > >>>> +++ b/src/android/camera_device.cpp > >>>> @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer > >>>> continue; > >>>> } > >>>> > >>>> - /* > >>>> - * Setting length to zero here is OK as the length is only used > >>>> - * to map the memory of the plane. Libcamera do not need to poke > >>>> - * at the memory content queued by the HAL. > >>>> - */ > >>>> - plane.length = 0; > >>>> + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); > >>>> + if (length == -1) { > >>>> + /* > >>>> + * A zero length plane is not fatal unless the > >>>> + * FrameBuffer is used for a software stream, libcamera > >>>> + * itself will not access the internal frame content. > >>>> + */ > >>>> + LOG(HAL, Error) << "Failed to query plane length"; > >>>> + length = 0; > >>> > >>> Do you expect this to happen ? If not, I'd return an error. If yes, an > >>> error message would be printed in a loop, which isn't very nice. My > >>> preference would be to return an error if possible. > >>> > > > > I don't think I expect it to happen. > > > >>> Otherwise, nice hack :-) > >> > >> A zero-length plane would be strange and could suggest that the > >> backing file is not a DMA-buf, so it could indeed make sense to error > >> out. > > > > I suspect I coded this thinking that 0 was currently acceptable, so it > > might still be acceptable to use. > > > > But I agree with you both that if we can't determine the length of the > > buffer then we have an error and should highlight it accordingly. > > > >> Otherwise, we tend to do this in a number of places in Chrome OS and > >> seems to be the official way to query a DMA-buf for its size. However > >> I wonder if we shouldn't restore the original position after the seek? > > > > Does CrOS do this? It's an extra couple of calls, (SEEK_CUR, and > > SEEK_SET), but I hope they're not too expensive ... > > > > I would expect anyone using a DMABuf will mmap the data if needed rather > > than use read()/write(), but that's probably making too big an > > assumption, so indeed if we left it at SEEK_END we might break something ... > > The views on this are separated even in Chrome OS. There are places > where we simply set it back to 0 and there are places where we do a > proper save, change and restore. I don't have a strong opinion on this > and I'm pretty much sure we don't have any read/write calls to > DMA-bufs in Chrome OS, but there could be some implications on other > platforms, so I've intentionally made the last sentence a question. :) dmabuf doesn't support read/write, so I think it's safe to leave it as-is.
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 538b8ab5da03..6212ccdd61ec 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer continue; } - /* - * Setting length to zero here is OK as the length is only used - * to map the memory of the plane. Libcamera do not need to poke - * at the memory content queued by the HAL. - */ - plane.length = 0; + off_t length = lseek(plane.fd.fd(), 0, SEEK_END); + if (length == -1) { + /* + * A zero length plane is not fatal unless the + * FrameBuffer is used for a software stream, libcamera + * itself will not access the internal frame content. + */ + LOG(HAL, Error) << "Failed to query plane length"; + length = 0; + } + + plane.length = length; planes.push_back(std::move(plane)); }
Use lseek to query the length of planes where possible rather than leaving the plane.length as zero, which prevents mapping buffers for software processing. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- I like this one ;-) We need to know the plane lengths for software streams now, but the correct way to get this would be to talk to the gralloc allocator. This would mean pulling in various extra cros-specific libraries, where instead we can query the size of the dmabuf by getting the offset when we SEEK_END. src/android/camera_device.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)