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
