[libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset in mapping IPABuffer

[Hirokazu Honda]

Hi Kieran,


On Wed, Aug 25, 2021 at 5:06 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 25/08/2021 07:13, Hirokazu Honda wrote:
> > Hi Laurent and Kieran, thank you for reviewing.
>
> <snip>
>
> >>>
> >>> We could use emplace() here to avoid the need for a default constructor,
> >>> but we wouldn't be able to use operator[]() below, which isn't nice. I
> >>> wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in
> >>> mappedBuffers_, to avoid the default constructor ?
> >>>
> >>> Kieran, you've designed the MappedFrameBuffer class, what do you think ?
> >>
> >> Hrm, firstly the error_ flag needs to be fixed up if there's a default
> >> constructor - mentioned below in the Constructor code.
> >>
> >> But otherwise, even though we do have the isValid() check, I don't think
> >> we have many users that actually check it.
> >>
> >> The aim was to make it simple and easy to create the mapping, which is
> >> why it happens on construction.
> >>
> >> If this is causing problems, we can separate construction and mapping...
> >> but then I'd expect to have map()/unmap() as well.
> >>
> >>
> >> Looking at existing users of MappedFrameBuffer, I can see that:
> >>
> >>  IPAVimc::mapBuffers() currently uses emplace, (with a
> >> piecewise_construct, to be able to specify the mapping flags as
> >> read-only), and only uses find() to get them.
> >>
> >>
> >> So does IPU3. ...
> >>
> >> And unsurprisingly, so does RPi.
> >>
> >>
> >> However none of those implementations are checking the isValid() flag.
> >> So perhaps they need to be updated to do so.
> >>
> >> And if we mandate that the isValid() should be checked, then it's not
> >> much different to separating construction and mapping...
> >>
> >>
> >> The main goal for the current design is to allow other buffer types to
> >> be constructed, so perhaps there could be a MappedDRMBuffer or
> >> MappedDMABuffer or such.
> >>
> >> As long as that's kept in mind, I don't mind if the main interface
> >> changes to suit needs as we determine them.
> >>
> >> Would using [], require checking the .isValid() flag at every usage?
> >> (Because we might be given a new empty mapping, if the search failed?)
> >>
> >> That might become less convenient ... ?
> >>
> >>
> >
> > I store std::unique_ptr<MappedFrameBuffer> to map as Laurent suggested.
> > So I can remove the default constructor of MappedFrameBuffer.
>
> Do we need to change all the other IPA's to use the same pattern ?
>

Yes, that should be good.
Which do you prefer, storing MappedFrameBuffer as unique_ptr or having
MappedFrameBuffer constructor?

-Hiro
> --
> Kieran
>