Re: [PATCH v1 3/5] media: stm32-dcmipp: STM32 DCMIPP camera interface driver

[Sakari Ailus]

Hi Alain,

On Fri, Aug 25, 2023 at 01:09:03PM +0200, Alain Volmat wrote:
...
> > > +static int dcmipp_pipeline_s_stream(struct dcmipp_bytecap_device *vcap,
> > > +				    int state)
> > > +{
> > > +	struct media_entity *entity = &vcap->vdev.entity;
> > > +	struct v4l2_subdev *subdev;
> > > +	struct media_pad *pad;
> > > +	int ret;
> > > +
> > > +	/* Start/stop all entities within pipeline */
> > > +	while (1) {
> > > +		pad = &entity->pads[0];
> > > +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > > +			break;
> > > +
> > > +		pad = media_pad_remote_pad_first(pad);
> > > +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> > > +			break;
> > > +
> > > +		entity = pad->entity;
> > > +		subdev = media_entity_to_v4l2_subdev(entity);
> > > +
> > > +		ret = v4l2_subdev_call(subdev, video, s_stream, state);
> > 
> > Does this driver handle multiple sub-devices in the same pipeline?
> > 
> > If not, then you don't need a loop here.
> 
> The idea was to enable one after the other each subdevs part of the
> pipeline (aka: sensor -> bridge -> parallel -> byteproc -> bytecap)
> however following a discussion with Laurent in Prague I changed that
> so that each subdev call each other in cascade, quite like I already did
> the following patch for the dcmi driver:

Ack!

> 
> commit 525011d84a3f547d0643c10bbfc01d32e26a9963
> Author: Alain Volmat <alain.volmat@xxxxxxxxxxx>
> Date:   Fri Jul 21 14:03:15 2023 +0200
> 
>     media: stm32: dcmi: only call s_stream on the source subdev
> 
>     Avoid calling s_stream on each subdev until reaching the sensor and
>     instead call s_stream on the source subdev only (which will in turn
>     do whatever needed to start the stream).
> 
>     Signed-off-by: Alain Volmat <alain.volmat@xxxxxxxxxxx>
>     Reviewed-by: Hugues FRUCHET <hugues.fruchet@xxxxxxxxxxx>
>     Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>

...

> > > +#define STOP_TIMEOUT_US 1000
> > > +#define POLL_INTERVAL_US  50
> > > +static int dcmipp_byteproc_s_stream(struct v4l2_subdev *sd, int enable)
> > > +{
> > > +	struct dcmipp_byteproc_device *byteproc = v4l2_get_subdevdata(sd);
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&byteproc->lock);
> > > +	if (enable) {
> > > +		dcmipp_byteproc_configure_framerate(byteproc);
> > > +
> > > +		ret = dcmipp_byteproc_configure_scale_crop(byteproc);
> > > +		if (ret)
> > > +			goto err;
> > 
> > This does nothing.
> 
> Not sure to understand your point here.  The s_stream callback of this
> subdev is used to configure the registers (here the ones controlling
> decimation and cropping) of the byteproc subdev.

I was referring to the last two lines --- you're jumping to essentially the
same location here.

> 
> > 
> > > +	}
> > > +
> > > +err:
> > > +	mutex_unlock(&byteproc->lock);
> > > +
> > > +	return ret;
> > > +}

...

> > > diff --git a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c
> > > new file mode 100644
> > > index 000000000000..aa7ae9a5b1a8
> > > --- /dev/null
> > > +++ b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c
> > > @@ -0,0 +1,682 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for STM32 Digital Camera Memory Interface Pixel Processor
> > > + *
> > > + * Copyright (C) STMicroelectronics SA 2022
> > > + * Authors: Hugues Fruchet <hugues.fruchet@xxxxxxxxxxx>
> > > + *          Alain Volmat <alain.volmat@xxxxxxxxxxx>
> > > + *          for STMicroelectronics.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/component.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/init.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_graph.h>
> > 
> > #include <linux/property.h> instead of these three.
> 
> Added linux/property.h however kept of_graph.h which is still necessary.
> 

You should switch to fwnode graph API as you're already using fwnodes in
the driver --- due to V4L2 fwnode.

...

> > > +static int dcmipp_graph_notify_bound(struct v4l2_async_notifier *notifier,
> > > +				     struct v4l2_subdev *subdev,
> > > +				     struct v4l2_async_subdev *asd)
> > > +{
> > > +	struct dcmipp_device *dcmipp = notifier_to_dcmipp(notifier);
> > > +	unsigned int ret;
> > > +	int src_pad;
> > > +	struct dcmipp_ent_device *sink;
> > > +	struct device_node *np = dcmipp->dev->of_node;
> > > +	struct v4l2_fwnode_endpoint ep = { .bus_type = 0 };
> > 
> > Please set bus_type explicitly (DPHY)?
> 
> My understanding is that I cannot set the bus_type here to have the
> framework check for me since we support both V4L2_MBUS_PARALLEL and
> V4L2_MBUS_BT656.

Ah, I missed this was using a parallel bus.

As you have a default in bindings, then you'll need to parse this assuming
that bus-type first. I.e. set the bus type to the default and if parsing
fails, try the other one.

-- 
Kind regards,

Sakari Ailus