linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hugues FRUCHET <hugues.fruchet@st.com>
To: Hans Verkuil <hverkuil@xs4all.nl>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Alexandre TORGUE <alexandre.torgue@st.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"linux-stm32@st-md-mailman.stormreply.com" 
	<linux-stm32@st-md-mailman.stormreply.com>,
	Yannick FERTRE <yannick.fertre@st.com>,
	Philippe CORNU <philippe.cornu@st.com>,
	"Mickael GUENE" <mickael.guene@st.com>
Subject: Re: [PATCH v3 3/3] media: stm32-dcmi: add support of several sub-devices
Date: Wed, 31 Jul 2019 13:05:14 +0000	[thread overview]
Message-ID: <724dc1a9-0e4e-180f-9e52-a07904c98360@st.com> (raw)
In-Reply-To: <7c18a314-851b-2852-83be-406cdc2f2047@xs4all.nl>

Hi Hans,

I've just pushed v4 with subdev nodes registry removal as per your 
suggestion.

On 7/25/19 5:09 PM, Hans Verkuil wrote:
> On 7/25/19 4:56 PM, Benjamin Gaignard wrote:
>> 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).
> 
> Yes, and I agree completely. But why then do you register v4l-subdevX devices?
> That's not needed for this driver.
> 
> Regards,
> 
> 	Hans
> 
>>
>> 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;
>>>>   }
>>>>
>>>>
>>>
> 

Best regards,
Hugues.

      reply	other threads:[~2019-07-31 13:05 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
2019-07-25 15:09       ` Hans Verkuil
2019-07-31 13:05         ` Hugues FRUCHET [this message]

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=724dc1a9-0e4e-180f-9e52-a07904c98360@st.com \
    --to=hugues.fruchet@st.com \
    --cc=alexandre.torgue@st.com \
    --cc=benjamin.gaignard@linaro.org \
    --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).