From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Alexandre Torgue <alexandre.torgue@st.com>,
Mickael GUENE <mickael.guene@st.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Philippe CORNU <philippe.cornu@st.com>,
Yannick Fertre <yannick.fertre@st.com>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Hugues Fruchet <hugues.fruchet@st.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-stm32@st-md-mailman.stormreply.com,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v3 3/3] media: stm32-dcmi: add support of several sub-devices
Date: Thu, 25 Jul 2019 16:56:31 +0200 [thread overview]
Message-ID: <CA+M3ks7HRc2C+dFpS9AM8ANo+=f360SFTCRrjavbqjd_SWkr_A@mail.gmail.com> (raw)
In-Reply-To: <81e1a94d-af25-302c-64a6-3cec096d4144@xs4all.nl>
Le jeu. 25 juil. 2019 à 13:40, Hans Verkuil <hverkuil@xs4all.nl> a écrit :
>
> On 7/2/19 5:52 PM, Hugues Fruchet wrote:
> > Add support of several sub-devices within pipeline instead
> > of a single one.
> > This allows to support a CSI-2 camera sensor connected
> > through a CSI-2 to parallel bridge.
> >
> > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > ---
> > drivers/media/platform/stm32/stm32-dcmi.c | 204 +++++++++++++++++++++++++++---
> > 1 file changed, 186 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> > index 6f37617..6921e6b 100644
> > --- a/drivers/media/platform/stm32/stm32-dcmi.c
> > +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> > @@ -172,6 +172,7 @@ struct stm32_dcmi {
> >
> > struct media_device mdev;
> > struct media_pad vid_cap_pad;
> > + struct media_pipeline pipeline;
> > };
> >
> > static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
> > @@ -583,6 +584,131 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
> > spin_unlock_irq(&dcmi->irqlock);
> > }
> >
> > +static struct media_entity *dcmi_find_source(struct stm32_dcmi *dcmi)
> > +{
> > + struct media_entity *entity = &dcmi->vdev->entity;
> > + struct media_pad *pad;
> > +
> > + /* Walk searching for entity having no sink */
> > + while (1) {
> > + pad = &entity->pads[0];
> > + if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > + break;
> > +
> > + pad = media_entity_remote_pad(pad);
> > + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> > + break;
> > +
> > + entity = pad->entity;
> > + }
> > +
> > + return entity;
> > +}
> > +
> > +static int dcmi_pipeline_s_fmt(struct stm32_dcmi *dcmi,
> > + struct v4l2_subdev_pad_config *pad_cfg,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct media_entity *entity = &dcmi->entity.source->entity;
> > + struct v4l2_subdev *subdev;
> > + struct media_pad *sink_pad = NULL;
> > + struct media_pad *src_pad = NULL;
> > + struct media_pad *pad = NULL;
> > + struct v4l2_subdev_format fmt = *format;
> > + bool found = false;
> > + int ret;
> > +
> > + /*
> > + * Starting from sensor subdevice, walk within
> > + * pipeline and set format on each subdevice
> > + */
> > + while (1) {
> > + unsigned int i;
> > +
> > + /* Search if current entity has a source pad */
> > + for (i = 0; i < entity->num_pads; i++) {
> > + pad = &entity->pads[i];
> > + if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> > + src_pad = pad;
> > + found = true;
> > + break;
> > + }
> > + }
> > + if (!found)
> > + break;
> > +
> > + subdev = media_entity_to_v4l2_subdev(entity);
> > +
> > + /* Propagate format on sink pad if any, otherwise source pad */
> > + if (sink_pad)
> > + pad = sink_pad;
> > +
> > + dev_dbg(dcmi->dev, "%s[%d] pad format set to 0x%x %ux%u\n",
> > + subdev->name, pad->index, format->format.code,
> > + format->format.width, format->format.height);
> > +
> > + fmt.pad = pad->index;
> > + ret = v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Walk to next entity */
> > + sink_pad = media_entity_remote_pad(src_pad);
> > + if (!sink_pad || !is_media_entity_v4l2_subdev(sink_pad->entity))
> > + break;
> > +
> > + entity = sink_pad->entity;
> > + }
> > + *format = fmt;
> > +
> > + return 0;
> > +}
> > +
> > +static int dcmi_pipeline_s_stream(struct stm32_dcmi *dcmi, int state)
> > +{
> > + struct media_entity *entity = &dcmi->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_entity_remote_pad(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);
> > + if (ret < 0 && ret != -ENOIOCTLCMD) {
> > + dev_err(dcmi->dev, "%s: %s failed to %s streaming (%d)\n",
> > + __func__, subdev->name,
> > + state ? "start" : "stop", ret);
> > + return ret;
> > + }
> > +
> > + dev_dbg(dcmi->dev, "%s is %s\n",
> > + subdev->name, state ? "started" : "stopped");
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int dcmi_pipeline_start(struct stm32_dcmi *dcmi)
> > +{
> > + return dcmi_pipeline_s_stream(dcmi, 1);
> > +}
> > +
> > +static void dcmi_pipeline_stop(struct stm32_dcmi *dcmi)
> > +{
> > + dcmi_pipeline_s_stream(dcmi, 0);
> > +}
> > +
> > static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
> > {
> > struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
> > @@ -597,14 +723,17 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
> > goto err_release_buffers;
> > }
> >
> > - /* Enable stream on the sub device */
> > - ret = v4l2_subdev_call(dcmi->entity.source, video, s_stream, 1);
> > - if (ret && ret != -ENOIOCTLCMD) {
> > - dev_err(dcmi->dev, "%s: Failed to start streaming, subdev streamon error",
> > - __func__);
> > + ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
> > + if (ret < 0) {
> > + dev_err(dcmi->dev, "%s: Failed to start streaming, media pipeline start error (%d)\n",
> > + __func__, ret);
> > goto err_pm_put;
> > }
> >
> > + ret = dcmi_pipeline_start(dcmi);
> > + if (ret)
> > + goto err_media_pipeline_stop;
> > +
> > spin_lock_irq(&dcmi->irqlock);
> >
> > /* Set bus width */
> > @@ -676,7 +805,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
> > if (ret) {
> > dev_err(dcmi->dev, "%s: Start streaming failed, cannot start capture\n",
> > __func__);
> > - goto err_subdev_streamoff;
> > + goto err_pipeline_stop;
> > }
> >
> > /* Enable interruptions */
> > @@ -687,8 +816,11 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
> >
> > return 0;
> >
> > -err_subdev_streamoff:
> > - v4l2_subdev_call(dcmi->entity.source, video, s_stream, 0);
> > +err_pipeline_stop:
> > + dcmi_pipeline_stop(dcmi);
> > +
> > +err_media_pipeline_stop:
> > + media_pipeline_stop(&dcmi->vdev->entity);
> >
> > err_pm_put:
> > pm_runtime_put(dcmi->dev);
> > @@ -713,13 +845,10 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
> > {
> > struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
> > struct dcmi_buf *buf, *node;
> > - int ret;
> >
> > - /* Disable stream on the sub device */
> > - ret = v4l2_subdev_call(dcmi->entity.source, video, s_stream, 0);
> > - if (ret && ret != -ENOIOCTLCMD)
> > - dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
> > - __func__, ret);
> > + dcmi_pipeline_stop(dcmi);
> > +
> > + media_pipeline_stop(&dcmi->vdev->entity);
> >
> > spin_lock_irq(&dcmi->irqlock);
> >
> > @@ -937,8 +1066,7 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
> > mf->width = sd_framesize.width;
> > mf->height = sd_framesize.height;
> >
> > - ret = v4l2_subdev_call(dcmi->entity.source, pad,
> > - set_fmt, NULL, &format);
> > + ret = dcmi_pipeline_s_fmt(dcmi, NULL, &format);
> > if (ret < 0)
> > return ret;
> >
> > @@ -1529,7 +1657,20 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
> > struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
> > int ret;
> >
> > + /*
> > + * Now that the graph is complete,
> > + * we search for the source subdevice
> > + * in order to expose it through V4L2 interface
> > + */
> > + dcmi->entity.source =
> > + media_entity_to_v4l2_subdev(dcmi_find_source(dcmi));
> > + if (!dcmi->entity.source) {
> > + dev_err(dcmi->dev, "Source subdevice not found\n");
> > + return -ENODEV;
> > + }
> > +
> > dcmi->vdev->ctrl_handler = dcmi->entity.source->ctrl_handler;
> > +
> > ret = dcmi_formats_init(dcmi);
> > if (ret) {
> > dev_err(dcmi->dev, "No supported mediabus format found\n");
> > @@ -1574,12 +1715,30 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
> > struct v4l2_async_subdev *asd)
> > {
> > struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
> > + unsigned int ret;
> > + int src_pad;
> >
> > dev_dbg(dcmi->dev, "Subdev %s bound\n", subdev->name);
> >
> > - dcmi->entity.source = subdev;
> > + /*
> > + * Link this sub-device to DCMI, it could be
> > + * a parallel camera sensor or a bridge
> > + */
> > + src_pad = media_entity_get_fwnode_pad(&subdev->entity,
> > + subdev->fwnode,
> > + MEDIA_PAD_FL_SOURCE);
> > +
> > + ret = media_create_pad_link(&subdev->entity, src_pad,
> > + &dcmi->vdev->entity, 0,
> > + MEDIA_LNK_FL_IMMUTABLE |
> > + MEDIA_LNK_FL_ENABLED);
> > + if (ret)
> > + dev_err(dcmi->dev, "Failed to create media pad link with subdev %s\n",
> > + subdev->name);
> > + else
> > + dev_dbg(dcmi->dev, "DCMI is now linked to %s\n", subdev->name);
> >
> > - return 0;
> > + return ret;
> > }
> >
> > static const struct v4l2_async_notifier_operations dcmi_graph_notify_ops = {
> > @@ -1639,6 +1798,15 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
> > return ret;
> > }
> >
> > + /* Register all the subdev nodes */
> > + ret = v4l2_device_register_subdev_nodes(&dcmi->v4l2_dev);
>
> This shouldn't be needed. Only MC-centric drivers (i.e. where the pipeline
> has to be configured by userspace) need to do this.
Hi Hans,
I think this point has been discussed in this thread
https://www.spinics.net/lists/linux-media/msg153417.html
In short : since the hardware only offer one possible path we don't expose
the configuration to userland and let DCMI driver configure the
subdevice (like bridge).
Benjamin
>
> Otherwise this patch looks good.
>
> Regards,
>
> Hans
>
> > + if (ret) {
> > + dev_err(dcmi->dev, "Failed to register subdev nodes\n");
> > + v4l2_async_notifier_unregister(&dcmi->notifier);
> > + of_node_put(dcmi->entity.remote_node);
> > + return ret;
> > + }
> > +
> > return 0;
> > }
> >
> >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-07-25 14:57 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-02 15:52 [PATCH v3 0/3] DCMI bridge support Hugues Fruchet
2019-07-02 15:52 ` [PATCH v3 1/3] media: stm32-dcmi: improve sensor subdev naming Hugues Fruchet
2019-07-02 15:52 ` [PATCH v3 2/3] media: stm32-dcmi: add media controller support Hugues Fruchet
2019-07-02 15:52 ` [PATCH v3 3/3] media: stm32-dcmi: add support of several sub-devices Hugues Fruchet
2019-07-25 11:40 ` Hans Verkuil
2019-07-25 14:56 ` Benjamin Gaignard [this message]
2019-07-25 15:09 ` Hans Verkuil
2019-07-31 13:05 ` Hugues FRUCHET
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+M3ks7HRc2C+dFpS9AM8ANo+=f360SFTCRrjavbqjd_SWkr_A@mail.gmail.com' \
--to=benjamin.gaignard@linaro.org \
--cc=alexandre.torgue@st.com \
--cc=hugues.fruchet@st.com \
--cc=hverkuil@xs4all.nl \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mchehab@kernel.org \
--cc=mickael.guene@st.com \
--cc=philippe.cornu@st.com \
--cc=sakari.ailus@linux.intel.com \
--cc=yannick.fertre@st.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).