[libcamera-devel] [PATCH] libcamera: pipeline: simple: Add support for ST's DCMIPP

[Alain Volmat]

Hi Laurent,

On Wed, May 03, 2023 at 03:42:45PM +0300, Laurent Pinchart wrote:
> Hi Hugues,
> 
> On Wed, May 03, 2023 at 10:18:01AM +0200, Hugues FRUCHET wrote:
> > Hi Laurent,
> > 
> > DCMIPP kernel driver has been submitted but no feedback received yet:
> > https://www.spinics.net/lists/linux-media/msg218378.html
> > Would be nice to get at least a first level of review before pushing a 
> > new version.
> 
> OK, I'll take the blame :-) Dan told me he will review the patch series,
> I'll try to do so too, but will possibly wait for a v2 if he reports
> many issues to be addressed.

Thanks a lot.  Once we have some first comments I will push a v2 :-)

> > For GC2145, we are in the middle of the bridge with two versions of 
> > driver that can be found on the net: a pinephone version  with a 
> > parallel interface support and our driver which is a CSI-2 version (we 
> > have a small amount of support from GalaxyCore, not amazing but better 
> > than nothing...)
> 
> Do you plan to upstream your driver ?

The idea is to first give a try to merge our driver with the one from
pinephone (that is adding the CSI2 part into the pinephone version).
I am giving it a try currently and then yes, for sure I'll then upstream
the driver.

> 
> > On 5/2/23 14:33, Laurent Pinchart wrote:
> > > On Tue, May 02, 2023 at 01:29:31PM +0100, Kieran Bingham via libcamera-devel wrote:
> > >> Quoting Daniel Scally via libcamera-devel (2023-05-02 12:13:57)
> > >>> The STM32 contains a camera pipeline known as the DCMIPP (Digital
> > >>> Camera-Memory Interface Pixel Processor) which receives data from a
> > >>> parallel interface and dumps the post-processed data to memory. The
> > >>> pipeline is capable of some processing in the form of downscaling
> > >>> captured data through cropping or skipping the sensor's output.
> > >>>
> > >>> The simple pipeline handler is quite capable of handling the DCMIPP
> > >>> given its operation is handled entirely through configuring the pads
> > >>> of a media graph, so add support for the driver to the pipeline's
> > >>> supportedDevices array.
> > >>>
> > >>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > >>> ---
> > >>> Support for the dcmipp has been tested on an stm32mp135f-dk with a gc2145
> > >>> camera sensor. Note that the dcmipp driver **is not** in mainline at the moment,
> > >>> and nor is the gc2145's driver. @Hugues, @Alain, is there a plan to move those
> > >>> drivers upstream?
> > >>
> > >> As I understand it, the drivers are open, and as long as there is an
> > >> expectation that they are going upstream - that's 'enough' to get this
> > >> merged in libcamera I believe.
> > > 
> > > We don't have to wait for the dcmipp driver to land upstream before
> > > merging this, but I'd like to understand the upstreaming plan first.
> > > 
> > >> It's also a really easily 'line' to maintain ;-)
> > >>
> > >>> A few changes to ST's kernel were needed to fix some format propagation problems
> > >>> and some missing controls for the GC2145 for this to work. Those can be found in
> > >>> my tree linked below [1], along with some fixes to the CSI-2 to Parallel bridge
> > >>> driver which _is_ upstream and which I posted to linux-media [2].
> > >>>
> > >>> [1] https://github.com/djrscally/linux/tree/v5.15-stm32mp
> > >>> [2] https://lore.kernel.org/linux-media/20230502103547.150918-1-dan.scally@ideasonboard.com/T/#t
> > >>>
> > >>>   src/libcamera/pipeline/simple/simple.cpp | 1 +
> > >>>   1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > >>> index e1f8b989..01516bc3 100644
> > >>> --- a/src/libcamera/pipeline/simple/simple.cpp
> > >>> +++ b/src/libcamera/pipeline/simple/simple.cpp
> > >>> @@ -194,6 +194,7 @@ static const SimplePipelineInfo supportedDevices[] = {
> > >>>          { "mxc-isi", {} },
> > >>>          { "qcom-camss", {} },
> > >>>          { "sun6i-csi", {} },
> > >>> +       { "dcmipp", {} },
> > >>
> > >> Alphabetical sort order ;-)
> > >>
> > >> With that...
> > >>
> > >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >>
> > >>>   };
> > >>>   
> > >>>   } /* namespace */
> 
> -- 
> Regards,
> 
> Laurent Pinchart