[libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test back channel
Jacopo Mondi
jacopo at jmondi.org
Fri Oct 4 10:31:15 CEST 2019
Hi Laurent,
On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote:
> > Add support to the dummy VIMC IPA for communication with the IPA
> > interface test unit.
> >
> > Use the test channel, if available, to send a confirmation code when
> > the init() operation is called.
>
> I would move this patch before 4/5, to avoid introducing a test that
> would fail in 4/5.
>
Yeah, better!
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 54 insertions(+)
> >
> > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp
> > index abc06e7f5fd5..781f5796b082 100644
> > --- a/src/ipa/ipa_vimc.cpp
> > +++ b/src/ipa/ipa_vimc.cpp
> > @@ -7,20 +7,74 @@
> >
> > #include <iostream>
> >
>
> No need for a blank line (and iostream should then go after fcntl.h).
>
Don't we keep c++ and c headers inclusion directives separate ?
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>
> > +
> > #include <ipa/ipa_interface.h>
> > #include <ipa/ipa_module_info.h>
> >
> > namespace libcamera {
> >
> > +/* Keep these in sync with the IPA Interface test unit. */
> > +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo";
>
> I wonder if the path to the FIFO should be passed to the IPA module
> through an environment variable.
>
I thought how to avoid having to keep the two in sync, including a
shared header
> > +#define IPA_INIT_CODE 0x01
>
> How about moving this to include/ipa/vimc.h ?
>
...as you suggest here. It would however contain mostly definitions for
the testing infrastructure, and I was not sure how welcome that would
be. I'll go ahead with this since it seems there are no objections
> > +
> > class IPAVimc : public IPAInterface
> > {
> > public:
> > + IPAVimc();
> > + ~IPAVimc();
> > +
> > int init();
> > +
> > +private:
> > + unsigned int fd_;
>
> fds are int, not unsigned int.
>
? Only if they are used to store error codes. Negative file
descriptors ?
> > };
> >
> > +IPAVimc::IPAVimc()
> > + : fd_(0)
>
> 0 is a valid value, it should be initialised to -1.
>
Are we using stdin ?
> > +{
> > + /* Set up the test unit back-channel, if available. */
> > + struct stat fifoStat;
> > + int ret = stat(vimcFifoPath, &fifoStat);
> > + if (ret)
> > + return;
> > +
> > + ret = ::open(vimcFifoPath, O_WRONLY);
> > + if (ret < 0) {
> > + std::cerr << "Failed to open vimc IPA test fifo at: "
> > + << vimcFifoPath << ": " << strerror(errno)
> > + << std::endl;
>
> How about using LOG() ?
>
I saw cout/cerr where used an I thought IPA should avoid using LOG(),
but there are no reasons for doing so, as this IPA is linked against
libcamera anyhow
> > + return;
> > + }
> > + fd_ = ret;
> > +}
> > +
> > +IPAVimc::~IPAVimc()
> > +{
> > + if (fd_)
> > + ::close(fd_);
> > +}
> > +
> > int IPAVimc::init()
> > {
> > std::cout << "initializing vimc IPA!" << std::endl;
> > +
> > + if (!fd_)
> > + return 0;
> > +
> > + int8_t data = IPA_INIT_CODE;
> > + int ret = ::write(fd_, &data, 1);
> > + if (ret < 0) {
> > + ret = -errno;
> > + std::cerr << "Failed to write to vimc IPA test fifo at: "
> > + << vimcFifoPath << ": " << strerror(-ret)
> > + << std::endl;
> > + return ret;
> > + }
>
> This will be enough for now, but to prepare for the future, should the
> write to FIFO be moved to a separate method that will later be called
> from the other IPAInterface operations ? I would then turn IPA_INIT_CODE
> into an
>
> enum IPAMethod {
> IPAMethodInit,
> ...
Could we wait to see how the interface looks like before defining the
enum
I however agree a separate method makes sense already.
> };
>
> and call
>
> int IPAVimc::init()
> {
> trace(IPAMethodInit);
>
> std:cout << ...
> }
>
> (bikeshedding the trace method name is allowed :-)).
>
trace is fine, or 'report', 'signal'...
> > +
> > return 0;
> > }
> >
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191004/456ae606/attachment.sig>
More information about the libcamera-devel
mailing list