| Message ID | 20200628001532.2685967-1-niklas.soderlund@ragnatech.se |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas and everyone, On Sun, Jun 28, 2020 at 2:15 AM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi, > > This series do some cleanup of the IPU3 pipeline. It removes dead or > almost dead code, replacing it with something more coherent to what is > done elsewhere in the pipeline. It breaks out the ImgU to a separate cpp > and h file. > > It reworks a bit how the streams are identified and handled inside the > pipeline. When the pipeline was first developed only the output and > viewfinder streams where available, now we have a RAW stream in addition > to this. Some of the abstraction around streams and which hardware > resource they are backed by made sens in the earlier context but not so > much once RAW is added to the mix. The biggest difference in this rework > is that the ImgUDevice gets explicit functions to configure each of its > sink instead of having a generic one which depends which pointer is > passed to it. This makes reading code where RAW and ImgU streams are > mixed much nicer IMHO. CIO2 is quite a generic capture interface, which could be handled by some platform-independent code. Have we considered splitting the pipeline into a generic capture part and platform-specific processing part? I think it could be useful on its own, as the capture part of all the SoCs could be handled by generic code, but I also suspect that the ability to split the pipeline into multiple smaller pipelines and then join them together could be very useful when dealing with many SoC generations, since vendors quite often replace some parts of the pipeline with new hardware, while keeping the others unchanged. What do you think? Best regards, Tomasz > > Lastly some assumptions that buffers must be allocated at video nodes > that are not involved in the capture session are being challenged. This > was true a year ago but not any more it seems. Chancing this simplifies > the driver enormously and saves on memory that otherwise would be > wasted. I have really tried to force the end result to failed by > resetting the hardware between each test so that no video node > configuration from a previous sessions saves the day. > > I have for both sensors reset the hardware and then tested the following > capture combinations successfully. After the first capture session for > each one in the list that was done after a power cycle all the other > captures where tried in a semi random order and always succeeded. > > cam -c 1 -s role=viewfinder -C > cam -c 1 -s role=still -C > cam -c 1 -s role=still -s role=viewfinder -C > cam -c 1 -s role=still -s role=viewfinder -s role=stillraw -C > cam -c 1 -s role=stillraw -C > > Niklas Söderlund (13): > libcamera: ipu3: Remove unused name_ filed from IPU3Stream > libcamera: ipu3: Import instead of allocate statistic buffers > libcamera: ipu3: Always import buffers for ImgU sinks > libcamera: ipu3: Remove usage of IPU3CameraData from ImgUDevice > libcamera: ipu3: imgu: Move the ImgUDevice class to separate files > libcamera: ipu3: imgu: Do not cache index > libcamera: ipu3: imgu: Mark things that are internal as private > libcamera: ipu3: imgu: Use specific functions to configure each sink > libcamera: ipu3: Do not duplicate data in IPU3Stream > libcamera: ipu3: Remove the active flag from IPU3Stream > libcamera: ipu3: Remove IPU3Stream > libcamera: ipu3: imgu: Remove ImgUOutput > libcamera: ipu3: imgu: Use unique_ptr for video and subdevices > > src/libcamera/pipeline/ipu3/imgu.cpp | 353 +++++++++++++++ > src/libcamera/pipeline/ipu3/imgu.h | 87 ++++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 548 +++--------------------- > src/libcamera/pipeline/ipu3/meson.build | 1 + > 4 files changed, 505 insertions(+), 484 deletions(-) > create mode 100644 src/libcamera/pipeline/ipu3/imgu.cpp > create mode 100644 src/libcamera/pipeline/ipu3/imgu.h > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Tomasz, On 2020-06-29 11:55:47 +0200, Tomasz Figa wrote: > Hi Niklas and everyone, > > On Sun, Jun 28, 2020 at 2:15 AM Niklas Söderlund > <niklas.soderlund@ragnatech.se> wrote: > > > > Hi, > > > > This series do some cleanup of the IPU3 pipeline. It removes dead or > > almost dead code, replacing it with something more coherent to what is > > done elsewhere in the pipeline. It breaks out the ImgU to a separate cpp > > and h file. > > > > It reworks a bit how the streams are identified and handled inside the > > pipeline. When the pipeline was first developed only the output and > > viewfinder streams where available, now we have a RAW stream in addition > > to this. Some of the abstraction around streams and which hardware > > resource they are backed by made sens in the earlier context but not so > > much once RAW is added to the mix. The biggest difference in this rework > > is that the ImgUDevice gets explicit functions to configure each of its > > sink instead of having a generic one which depends which pointer is > > passed to it. This makes reading code where RAW and ImgU streams are > > mixed much nicer IMHO. > > CIO2 is quite a generic capture interface, which could be handled by > some platform-independent code. Have we considered splitting the > pipeline into a generic capture part and platform-specific processing > part? I think it could be useful on its own, as the capture part of > all the SoCs could be handled by generic code, but I also suspect that > the ability to split the pipeline into multiple smaller pipelines and > then join them together could be very useful when dealing with many > SoC generations, since vendors quite often replace some parts of the > pipeline with new hardware, while keeping the others unchanged. > > What do you think? I think it is a good idea and something that have been lurking in the back of my head for some time. I'm even entertaining the thought if we could levy the simple-pipeline handler here somehow. I still have to dig in the code to see what is possible and what is not, and what is a good idea and what is not :-) Right now tho I think the most important area to try and help out pipeline handlers is around the Request object and how internal buffers (RAW, Statistics and Parameters) are attached to it when for example the RAW buffer could come from the application or from an internal pool. This will be a common problem for all pipelines and it would be nice if the core could help out here. > > Best regards, > Tomasz > > > > > Lastly some assumptions that buffers must be allocated at video nodes > > that are not involved in the capture session are being challenged. This > > was true a year ago but not any more it seems. Chancing this simplifies > > the driver enormously and saves on memory that otherwise would be > > wasted. I have really tried to force the end result to failed by > > resetting the hardware between each test so that no video node > > configuration from a previous sessions saves the day. > > > > I have for both sensors reset the hardware and then tested the following > > capture combinations successfully. After the first capture session for > > each one in the list that was done after a power cycle all the other > > captures where tried in a semi random order and always succeeded. > > > > cam -c 1 -s role=viewfinder -C > > cam -c 1 -s role=still -C > > cam -c 1 -s role=still -s role=viewfinder -C > > cam -c 1 -s role=still -s role=viewfinder -s role=stillraw -C > > cam -c 1 -s role=stillraw -C > > > > Niklas Söderlund (13): > > libcamera: ipu3: Remove unused name_ filed from IPU3Stream > > libcamera: ipu3: Import instead of allocate statistic buffers > > libcamera: ipu3: Always import buffers for ImgU sinks > > libcamera: ipu3: Remove usage of IPU3CameraData from ImgUDevice > > libcamera: ipu3: imgu: Move the ImgUDevice class to separate files > > libcamera: ipu3: imgu: Do not cache index > > libcamera: ipu3: imgu: Mark things that are internal as private > > libcamera: ipu3: imgu: Use specific functions to configure each sink > > libcamera: ipu3: Do not duplicate data in IPU3Stream > > libcamera: ipu3: Remove the active flag from IPU3Stream > > libcamera: ipu3: Remove IPU3Stream > > libcamera: ipu3: imgu: Remove ImgUOutput > > libcamera: ipu3: imgu: Use unique_ptr for video and subdevices > > > > src/libcamera/pipeline/ipu3/imgu.cpp | 353 +++++++++++++++ > > src/libcamera/pipeline/ipu3/imgu.h | 87 ++++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 548 +++--------------------- > > src/libcamera/pipeline/ipu3/meson.build | 1 + > > 4 files changed, 505 insertions(+), 484 deletions(-) > > create mode 100644 src/libcamera/pipeline/ipu3/imgu.cpp > > create mode 100644 src/libcamera/pipeline/ipu3/imgu.h > > > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On Mon, Jun 29, 2020 at 12:22 PM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi Tomasz, > > On 2020-06-29 11:55:47 +0200, Tomasz Figa wrote: > > Hi Niklas and everyone, > > > > On Sun, Jun 28, 2020 at 2:15 AM Niklas Söderlund > > <niklas.soderlund@ragnatech.se> wrote: > > > > > > Hi, > > > > > > This series do some cleanup of the IPU3 pipeline. It removes dead or > > > almost dead code, replacing it with something more coherent to what is > > > done elsewhere in the pipeline. It breaks out the ImgU to a separate cpp > > > and h file. > > > > > > It reworks a bit how the streams are identified and handled inside the > > > pipeline. When the pipeline was first developed only the output and > > > viewfinder streams where available, now we have a RAW stream in addition > > > to this. Some of the abstraction around streams and which hardware > > > resource they are backed by made sens in the earlier context but not so > > > much once RAW is added to the mix. The biggest difference in this rework > > > is that the ImgUDevice gets explicit functions to configure each of its > > > sink instead of having a generic one which depends which pointer is > > > passed to it. This makes reading code where RAW and ImgU streams are > > > mixed much nicer IMHO. > > > > CIO2 is quite a generic capture interface, which could be handled by > > some platform-independent code. Have we considered splitting the > > pipeline into a generic capture part and platform-specific processing > > part? I think it could be useful on its own, as the capture part of > > all the SoCs could be handled by generic code, but I also suspect that > > the ability to split the pipeline into multiple smaller pipelines and > > then join them together could be very useful when dealing with many > > SoC generations, since vendors quite often replace some parts of the > > pipeline with new hardware, while keeping the others unchanged. > > > > What do you think? > > I think it is a good idea and something that have been lurking in the > back of my head for some time. I'm even entertaining the thought if we > could levy the simple-pipeline handler here somehow. I still have to dig > in the code to see what is possible and what is not, and what is a good > idea and what is not :-) > Cool. Please let me know if I could be useful in brainstorming various ideas. > Right now tho I think the most important area to try and help out > pipeline handlers is around the Request object and how internal buffers > (RAW, Statistics and Parameters) are attached to it when for example the > RAW buffer could come from the application or from an internal pool. > This will be a common problem for all pipelines and it would be nice if > the core could help out here. Agreed. It was more of just a long term note. I agree that this series makes for a good overall improvement on its own. > > > > > Best regards, > > Tomasz > > > > > > > > Lastly some assumptions that buffers must be allocated at video nodes > > > that are not involved in the capture session are being challenged. This > > > was true a year ago but not any more it seems. Chancing this simplifies > > > the driver enormously and saves on memory that otherwise would be > > > wasted. I have really tried to force the end result to failed by > > > resetting the hardware between each test so that no video node > > > configuration from a previous sessions saves the day. > > > > > > I have for both sensors reset the hardware and then tested the following > > > capture combinations successfully. After the first capture session for > > > each one in the list that was done after a power cycle all the other > > > captures where tried in a semi random order and always succeeded. > > > > > > cam -c 1 -s role=viewfinder -C > > > cam -c 1 -s role=still -C > > > cam -c 1 -s role=still -s role=viewfinder -C > > > cam -c 1 -s role=still -s role=viewfinder -s role=stillraw -C > > > cam -c 1 -s role=stillraw -C > > > > > > Niklas Söderlund (13): > > > libcamera: ipu3: Remove unused name_ filed from IPU3Stream > > > libcamera: ipu3: Import instead of allocate statistic buffers > > > libcamera: ipu3: Always import buffers for ImgU sinks > > > libcamera: ipu3: Remove usage of IPU3CameraData from ImgUDevice > > > libcamera: ipu3: imgu: Move the ImgUDevice class to separate files > > > libcamera: ipu3: imgu: Do not cache index > > > libcamera: ipu3: imgu: Mark things that are internal as private > > > libcamera: ipu3: imgu: Use specific functions to configure each sink > > > libcamera: ipu3: Do not duplicate data in IPU3Stream > > > libcamera: ipu3: Remove the active flag from IPU3Stream > > > libcamera: ipu3: Remove IPU3Stream > > > libcamera: ipu3: imgu: Remove ImgUOutput > > > libcamera: ipu3: imgu: Use unique_ptr for video and subdevices > > > > > > src/libcamera/pipeline/ipu3/imgu.cpp | 353 +++++++++++++++ > > > src/libcamera/pipeline/ipu3/imgu.h | 87 ++++ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 548 +++--------------------- > > > src/libcamera/pipeline/ipu3/meson.build | 1 + > > > 4 files changed, 505 insertions(+), 484 deletions(-) > > > create mode 100644 src/libcamera/pipeline/ipu3/imgu.cpp > > > create mode 100644 src/libcamera/pipeline/ipu3/imgu.h > > > > > > -- > > > 2.27.0 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hello, On Mon, Jun 29, 2020 at 12:29:40PM +0200, Tomasz Figa wrote: > On Mon, Jun 29, 2020 at 12:22 PM Niklas Söderlund wrote: > > On 2020-06-29 11:55:47 +0200, Tomasz Figa wrote: > > > On Sun, Jun 28, 2020 at 2:15 AM Niklas Söderlund wrote: > > > > > > > > Hi, > > > > > > > > This series do some cleanup of the IPU3 pipeline. It removes dead or > > > > almost dead code, replacing it with something more coherent to what is > > > > done elsewhere in the pipeline. It breaks out the ImgU to a separate cpp > > > > and h file. > > > > > > > > It reworks a bit how the streams are identified and handled inside the > > > > pipeline. When the pipeline was first developed only the output and > > > > viewfinder streams where available, now we have a RAW stream in addition > > > > to this. Some of the abstraction around streams and which hardware > > > > resource they are backed by made sens in the earlier context but not so > > > > much once RAW is added to the mix. The biggest difference in this rework > > > > is that the ImgUDevice gets explicit functions to configure each of its > > > > sink instead of having a generic one which depends which pointer is > > > > passed to it. This makes reading code where RAW and ImgU streams are > > > > mixed much nicer IMHO. > > > > > > CIO2 is quite a generic capture interface, which could be handled by > > > some platform-independent code. Have we considered splitting the > > > pipeline into a generic capture part and platform-specific processing > > > part? I think it could be useful on its own, as the capture part of > > > all the SoCs could be handled by generic code, but I also suspect that > > > the ability to split the pipeline into multiple smaller pipelines and > > > then join them together could be very useful when dealing with many > > > SoC generations, since vendors quite often replace some parts of the > > > pipeline with new hardware, while keeping the others unchanged. > > > > > > What do you think? > > > > I think it is a good idea and something that have been lurking in the > > back of my head for some time. I'm even entertaining the thought if we > > could levy the simple-pipeline handler here somehow. I still have to dig > > in the code to see what is possible and what is not, and what is a good > > idea and what is not :-) Likewise, I've been thinking we should handle it in a more generic way. I thought there would be a chance to share code with the RPi pipeline handler as it also handles a CSI-2 receiver and a mem-to-mem ISP, but the driver for the CSI-2 receiver is video-node-centric there, while CIO2 is MC-centric. I've thus postponed working on this until we get a second pipeline handler that could share code with IPU3. > Cool. Please let me know if I could be useful in brainstorming various > ideas. > > > Right now tho I think the most important area to try and help out > > pipeline handlers is around the Request object and how internal buffers > > (RAW, Statistics and Parameters) are attached to it when for example the > > RAW buffer could come from the application or from an internal pool. > > This will be a common problem for all pipelines and it would be nice if > > the core could help out here. > > Agreed. It was more of just a long term note. I agree that this series > makes for a good overall improvement on its own. > > > > > Lastly some assumptions that buffers must be allocated at video nodes > > > > that are not involved in the capture session are being challenged. This > > > > was true a year ago but not any more it seems. Chancing this simplifies > > > > the driver enormously and saves on memory that otherwise would be > > > > wasted. I have really tried to force the end result to failed by > > > > resetting the hardware between each test so that no video node > > > > configuration from a previous sessions saves the day. > > > > > > > > I have for both sensors reset the hardware and then tested the following > > > > capture combinations successfully. After the first capture session for > > > > each one in the list that was done after a power cycle all the other > > > > captures where tried in a semi random order and always succeeded. > > > > > > > > cam -c 1 -s role=viewfinder -C > > > > cam -c 1 -s role=still -C > > > > cam -c 1 -s role=still -s role=viewfinder -C > > > > cam -c 1 -s role=still -s role=viewfinder -s role=stillraw -C > > > > cam -c 1 -s role=stillraw -C > > > > > > > > Niklas Söderlund (13): > > > > libcamera: ipu3: Remove unused name_ filed from IPU3Stream > > > > libcamera: ipu3: Import instead of allocate statistic buffers > > > > libcamera: ipu3: Always import buffers for ImgU sinks > > > > libcamera: ipu3: Remove usage of IPU3CameraData from ImgUDevice > > > > libcamera: ipu3: imgu: Move the ImgUDevice class to separate files > > > > libcamera: ipu3: imgu: Do not cache index > > > > libcamera: ipu3: imgu: Mark things that are internal as private > > > > libcamera: ipu3: imgu: Use specific functions to configure each sink > > > > libcamera: ipu3: Do not duplicate data in IPU3Stream > > > > libcamera: ipu3: Remove the active flag from IPU3Stream > > > > libcamera: ipu3: Remove IPU3Stream > > > > libcamera: ipu3: imgu: Remove ImgUOutput > > > > libcamera: ipu3: imgu: Use unique_ptr for video and subdevices > > > > > > > > src/libcamera/pipeline/ipu3/imgu.cpp | 353 +++++++++++++++ > > > > src/libcamera/pipeline/ipu3/imgu.h | 87 ++++ > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 548 +++--------------------- > > > > src/libcamera/pipeline/ipu3/meson.build | 1 + > > > > 4 files changed, 505 insertions(+), 484 deletions(-) > > > > create mode 100644 src/libcamera/pipeline/ipu3/imgu.cpp > > > > create mode 100644 src/libcamera/pipeline/ipu3/imgu.h > > > >