| Message ID | 1432139980-12619-9-git-send-email-william.towle@codethink.co.uk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1YvMOP-0002lM-FV; Thu, 21 May 2015 11:03:53 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-8) with esmtp id 1YvMON-0006XV-l2; Thu, 21 May 2015 11:03:53 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753041AbbEUJDs (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 21 May 2015 05:03:48 -0400 Received: from 82-70-136-246.dsl.in-addr.zen.co.uk ([82.70.136.246]:56598 "EHLO xk120.dyn.ducie.codethink.co.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753988AbbEUJDT (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 21 May 2015 05:03:19 -0400 Received: from william by xk120.dyn.ducie.codethink.co.uk with local (Exim 4.80) (envelope-from <william.towle@codethink.co.uk>) id 1Yv72G-0004Bm-Kn; Wed, 20 May 2015 17:40:00 +0100 From: William Towle <william.towle@codethink.co.uk> To: linux-kernel@lists.codethink.co.uk, linux-media@vger.kernel.org Cc: g.liakhovetski@gmx.de, sergei.shtylyov@cogentembedded.com, hverkuil@xs4all.nl, rob.taylor@codethink.co.uk Subject: [PATCH 08/20] media: soc_camera pad-aware driver initialisation Date: Wed, 20 May 2015 17:39:28 +0100 Message-Id: <1432139980-12619-9-git-send-email-william.towle@codethink.co.uk> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1432139980-12619-1-git-send-email-william.towle@codethink.co.uk> References: <1432139980-12619-1-git-send-email-william.towle@codethink.co.uk> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2015.5.21.85416 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, REFERENCES 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __REFERENCES 0, __SANE_MSGID 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
William Towle
May 20, 2015, 4:39 p.m. UTC
Add detection of source pad number for drivers aware of the media controller API, so that soc_camera/rcar_vin can create device nodes to support a driver such as adv7604.c (for HDMI on Lager) underneath. Signed-off-by: William Towle <william.towle@codethink.co.uk> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk> --- drivers/media/platform/soc_camera/rcar_vin.c | 4 ++++ drivers/media/platform/soc_camera/soc_camera.c | 27 +++++++++++++++++++++++- include/media/soc_camera.h | 1 + 3 files changed, 31 insertions(+), 1 deletion(-)
Comments
Hi Hans, On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote: > On 05/20/2015 06:39 PM, William Towle wrote: > > Add detection of source pad number for drivers aware of the media > > controller API, so that soc_camera/rcar_vin can create device nodes > > to support a driver such as adv7604.c (for HDMI on Lager) underneath. > > > > Signed-off-by: William Towle <william.towle@codethink.co.uk> > > Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk> > > --- > > > > drivers/media/platform/soc_camera/rcar_vin.c | 4 ++++ > > drivers/media/platform/soc_camera/soc_camera.c | 27 ++++++++++++++++++- > > include/media/soc_camera.h | 1 + > > 3 files changed, 31 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c > > b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43 > > 100644 > > --- a/drivers/media/platform/soc_camera/rcar_vin.c > > +++ b/drivers/media/platform/soc_camera/rcar_vin.c > > @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct > > soc_camera_device *icd, unsigned int idx, > > struct v4l2_mbus_framefmt *mf = &fmt.format; > > struct v4l2_rect rect; > > struct device *dev = icd->parent; > > + struct media_pad *remote_pad; > > int shift; > > > > + remote_pad = media_entity_remote_pad( > > + &icd->vdev->entity.pads[0]); > > + fmt.pad = remote_pad->index; > > This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media > calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER. > > Unfortunately, if it is not defined, then you still have no way of finding > the source pad. > > Laurent, do you think if it would make sense to add a new subdev core op > that will return the default source pad (I'm saying 'default' in case there > are more) of a subdev? That way it can be used in non-MC drivers. We never > needed the source pad before, but now we do, and this op only needs to be > implemented if the default source pad != 0. I'm not too fond of that. Is there something wrong with the method implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER an issue ? > > ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt); > > if (ret < 0) > > return ret; > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c > > b/drivers/media/platform/soc_camera/soc_camera.c index d708df4..126d645 > > 100644 > > --- a/drivers/media/platform/soc_camera/soc_camera.c > > +++ b/drivers/media/platform/soc_camera/soc_camera.c > > @@ -1293,6 +1293,7 @@ static int soc_camera_probe_finish(struct > > soc_camera_device *icd) > > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > }; > > struct v4l2_mbus_framefmt *mf = &fmt.format; > > + int src_pad_idx = -1; > > int ret; > > > > sd->grp_id = soc_camera_grp_id(icd); > > @@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct > > soc_camera_device *icd) > > } > > > > /* At this point client .probe() should have run already */ > > - ret = soc_camera_init_user_formats(icd); > > + ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0); > > + if (!ret) { > > + for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads; > > + src_pad_idx++) > > + if (sd->entity.pads[src_pad_idx].flags > > + == MEDIA_PAD_FL_SOURCE) > > + break; > > + > > + if (src_pad_idx < sd->entity.num_pads) { > > + if (!media_entity_create_link( > > + &icd->vdev->entity, 0, > > + &sd->entity, src_pad_idx, > > + MEDIA_LNK_FL_IMMUTABLE | > > + MEDIA_LNK_FL_ENABLED)) { > > + ret = soc_camera_init_user_formats(icd); > > + } > > + } > > + } > > + > > if (ret < 0) > > goto eusrfmt; > > > > @@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct > > soc_camera_device *icd) > > goto evidstart; > > > > /* Try to improve our guess of a reasonable window format */ > > + fmt.pad = src_pad_idx; > > if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) { > > icd->user_width = mf->width; > > icd->user_height = mf->height; > > @@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct > > soc_camera_device *icd) > > evidstart: > > soc_camera_free_user_formats(icd); > > > > eusrfmt: > > + media_entity_cleanup(&icd->vdev->entity); > > soc_camera_remove_device(icd); > > > > return ret; > > @@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct > > soc_camera_device *icd)> > > if (icd->num_user_formats) > > soc_camera_free_user_formats(icd); > > > > + if (icd->vdev->entity.num_pads) { > > + media_entity_cleanup(&icd->vdev->entity); > > + } > > + > > if (icd->clk) { > > /* For the synchronous case */ > > v4l2_clk_unregister(icd->clk); > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h > > index 2f6261f..f0c5238 100644 > > --- a/include/media/soc_camera.h > > +++ b/include/media/soc_camera.h > > @@ -42,6 +42,7 @@ struct soc_camera_device { > > unsigned char devnum; /* Device number per host */ > > struct soc_camera_sense *sense; /* See comment in struct definition > > */ > > struct video_device *vdev; > > + struct media_pad pad; > > struct v4l2_ctrl_handler ctrl_handler; > > const struct soc_camera_format_xlate *current_fmt; > > struct soc_camera_format_xlate *user_formats;
On 05/23/2015 08:32 PM, Laurent Pinchart wrote: > Hi Hans, > > On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote: >> On 05/20/2015 06:39 PM, William Towle wrote: >>> Add detection of source pad number for drivers aware of the media >>> controller API, so that soc_camera/rcar_vin can create device nodes >>> to support a driver such as adv7604.c (for HDMI on Lager) underneath. >>> >>> Signed-off-by: William Towle <william.towle@codethink.co.uk> >>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk> >>> --- >>> >>> drivers/media/platform/soc_camera/rcar_vin.c | 4 ++++ >>> drivers/media/platform/soc_camera/soc_camera.c | 27 ++++++++++++++++++- >>> include/media/soc_camera.h | 1 + >>> 3 files changed, 31 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c >>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43 >>> 100644 >>> --- a/drivers/media/platform/soc_camera/rcar_vin.c >>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c >>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct >>> soc_camera_device *icd, unsigned int idx, >>> struct v4l2_mbus_framefmt *mf = &fmt.format; >>> struct v4l2_rect rect; >>> struct device *dev = icd->parent; >>> + struct media_pad *remote_pad; >>> int shift; >>> >>> + remote_pad = media_entity_remote_pad( >>> + &icd->vdev->entity.pads[0]); >>> + fmt.pad = remote_pad->index; >> >> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media >> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER. >> >> Unfortunately, if it is not defined, then you still have no way of finding >> the source pad. >> >> Laurent, do you think if it would make sense to add a new subdev core op >> that will return the default source pad (I'm saying 'default' in case there >> are more) of a subdev? That way it can be used in non-MC drivers. We never >> needed the source pad before, but now we do, and this op only needs to be >> implemented if the default source pad != 0. > > I'm not too fond of that. Is there something wrong with the method implemented > in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER an issue ? 1) it's a heck of a lot of code just to get a simple source pad that the subdev knows anyway, 2) soc-camera doesn't use the media controller today, so this would add a dependency on the mc just for this, 3) it doesn't actually make a media device, it is just fakes enough to make the subdev think it there is an MC. It doesn't actually have to be a new op, it could be a new field in v4l2_subdev as well. For those bridge drivers that do not use the MC but do need to know the source pad this is important information. It might even simplify the device tree if the default source pad is implied unless stated otherwise (but that might be a step too far). I wonder if a default input pad might also be useful to expose. I suspect this might become important if we want to add MC support to all existing v4l2 drivers. Regards, Hans > >>> ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt); >>> if (ret < 0) >>> return ret; >>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c >>> b/drivers/media/platform/soc_camera/soc_camera.c index d708df4..126d645 >>> 100644 >>> --- a/drivers/media/platform/soc_camera/soc_camera.c >>> +++ b/drivers/media/platform/soc_camera/soc_camera.c >>> @@ -1293,6 +1293,7 @@ static int soc_camera_probe_finish(struct >>> soc_camera_device *icd) >>> .which = V4L2_SUBDEV_FORMAT_ACTIVE, >>> }; >>> struct v4l2_mbus_framefmt *mf = &fmt.format; >>> + int src_pad_idx = -1; >>> int ret; >>> >>> sd->grp_id = soc_camera_grp_id(icd); >>> @@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct >>> soc_camera_device *icd) >>> } >>> >>> /* At this point client .probe() should have run already */ >>> - ret = soc_camera_init_user_formats(icd); >>> + ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0); >>> + if (!ret) { >>> + for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads; >>> + src_pad_idx++) >>> + if (sd->entity.pads[src_pad_idx].flags >>> + == MEDIA_PAD_FL_SOURCE) >>> + break; >>> + >>> + if (src_pad_idx < sd->entity.num_pads) { >>> + if (!media_entity_create_link( >>> + &icd->vdev->entity, 0, >>> + &sd->entity, src_pad_idx, >>> + MEDIA_LNK_FL_IMMUTABLE | >>> + MEDIA_LNK_FL_ENABLED)) { >>> + ret = soc_camera_init_user_formats(icd); >>> + } >>> + } >>> + } >>> + >>> if (ret < 0) >>> goto eusrfmt; >>> >>> @@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct >>> soc_camera_device *icd) >>> goto evidstart; >>> >>> /* Try to improve our guess of a reasonable window format */ >>> + fmt.pad = src_pad_idx; >>> if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) { >>> icd->user_width = mf->width; >>> icd->user_height = mf->height; >>> @@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct >>> soc_camera_device *icd) >>> evidstart: >>> soc_camera_free_user_formats(icd); >>> >>> eusrfmt: >>> + media_entity_cleanup(&icd->vdev->entity); >>> soc_camera_remove_device(icd); >>> >>> return ret; >>> @@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct >>> soc_camera_device *icd)> >>> if (icd->num_user_formats) >>> soc_camera_free_user_formats(icd); >>> >>> + if (icd->vdev->entity.num_pads) { >>> + media_entity_cleanup(&icd->vdev->entity); >>> + } >>> + >>> if (icd->clk) { >>> /* For the synchronous case */ >>> v4l2_clk_unregister(icd->clk); >>> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h >>> index 2f6261f..f0c5238 100644 >>> --- a/include/media/soc_camera.h >>> +++ b/include/media/soc_camera.h >>> @@ -42,6 +42,7 @@ struct soc_camera_device { >>> unsigned char devnum; /* Device number per host */ >>> struct soc_camera_sense *sense; /* See comment in struct definition >>> */ >>> struct video_device *vdev; >>> + struct media_pad pad; >>> struct v4l2_ctrl_handler ctrl_handler; >>> const struct soc_camera_format_xlate *current_fmt; >>> struct soc_camera_format_xlate *user_formats; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Sunday 24 May 2015 10:08:15 Hans Verkuil wrote: > On 05/23/2015 08:32 PM, Laurent Pinchart wrote: > > On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote: > >> On 05/20/2015 06:39 PM, William Towle wrote: > >>> Add detection of source pad number for drivers aware of the media > >>> controller API, so that soc_camera/rcar_vin can create device nodes > >>> to support a driver such as adv7604.c (for HDMI on Lager) underneath. > >>> > >>> Signed-off-by: William Towle <william.towle@codethink.co.uk> > >>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk> > >>> --- > >>> > >>> drivers/media/platform/soc_camera/rcar_vin.c | 4 ++++ > >>> drivers/media/platform/soc_camera/soc_camera.c | 27 ++++++++++++++++- > >>> include/media/soc_camera.h | 1 + > >>> 3 files changed, 31 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c > >>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43 > >>> 100644 > >>> --- a/drivers/media/platform/soc_camera/rcar_vin.c > >>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c > >>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct > >>> soc_camera_device *icd, unsigned int idx, > >>> struct v4l2_mbus_framefmt *mf = &fmt.format; > >>> struct v4l2_rect rect; > >>> struct device *dev = icd->parent; > >>> + struct media_pad *remote_pad; > >>> int shift; > >>> > >>> + remote_pad = media_entity_remote_pad( > >>> + &icd->vdev->entity.pads[0]); > >>> + fmt.pad = remote_pad->index; > >> > >> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media > >> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER. > >> > >> Unfortunately, if it is not defined, then you still have no way of > >> finding the source pad. > >> > >> Laurent, do you think if it would make sense to add a new subdev core op > >> that will return the default source pad (I'm saying 'default' in case > >> there are more) of a subdev? That way it can be used in non-MC drivers. > >> We never needed the source pad before, but now we do, and this op only > >> needs to be implemented if the default source pad != 0. > > > > I'm not too fond of that. Is there something wrong with the method > > implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER > > an issue ? > > 1) it's a heck of a lot of code just to get a simple source pad that the > subdev knows anyway, I don't think the subdev knows. If a subdev has multiple source pads there's no concept of a default source. It all depends on how the subdevs are connected, and media_entity_remote_pad() is the right way to find out. > 2) soc-camera doesn't use the media controller today, so this would add a > dependency on the mc just for this, I agree that we shouldn't pull the whole MC userspace API in just for this, but the kernel side of the API should be available as pad-level operations depend on MC. We could split the CONFIG_MEDIA_CONTROLLER option in two. > 3) it doesn't actually make a media device, it is just fakes enough to make > the subdev think it there is an MC. > > It doesn't actually have to be a new op, it could be a new field in > v4l2_subdev as well. For those bridge drivers that do not use the MC but do > need to know the source pad this is important information. > > It might even simplify the device tree if the default source pad is implied > unless stated otherwise (but that might be a step too far). > > I wonder if a default input pad might also be useful to expose. I suspect > this might become important if we want to add MC support to all existing > v4l2 drivers. The concept of a default sink pad makes more sense, but I'm not sure I like it too much either. I'd have to think about it.
On 05/24/2015 11:50 PM, Laurent Pinchart wrote: > Hi Hans, > > On Sunday 24 May 2015 10:08:15 Hans Verkuil wrote: >> On 05/23/2015 08:32 PM, Laurent Pinchart wrote: >>> On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote: >>>> On 05/20/2015 06:39 PM, William Towle wrote: >>>>> Add detection of source pad number for drivers aware of the media >>>>> controller API, so that soc_camera/rcar_vin can create device nodes >>>>> to support a driver such as adv7604.c (for HDMI on Lager) underneath. >>>>> >>>>> Signed-off-by: William Towle <william.towle@codethink.co.uk> >>>>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk> >>>>> --- >>>>> >>>>> drivers/media/platform/soc_camera/rcar_vin.c | 4 ++++ >>>>> drivers/media/platform/soc_camera/soc_camera.c | 27 ++++++++++++++++- >>>>> include/media/soc_camera.h | 1 + >>>>> 3 files changed, 31 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c >>>>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43 >>>>> 100644 >>>>> --- a/drivers/media/platform/soc_camera/rcar_vin.c >>>>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c >>>>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct >>>>> soc_camera_device *icd, unsigned int idx, >>>>> struct v4l2_mbus_framefmt *mf = &fmt.format; >>>>> struct v4l2_rect rect; >>>>> struct device *dev = icd->parent; >>>>> + struct media_pad *remote_pad; >>>>> int shift; >>>>> >>>>> + remote_pad = media_entity_remote_pad( >>>>> + &icd->vdev->entity.pads[0]); >>>>> + fmt.pad = remote_pad->index; >>>> >>>> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media >>>> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER. >>>> >>>> Unfortunately, if it is not defined, then you still have no way of >>>> finding the source pad. >>>> >>>> Laurent, do you think if it would make sense to add a new subdev core op >>>> that will return the default source pad (I'm saying 'default' in case >>>> there are more) of a subdev? That way it can be used in non-MC drivers. >>>> We never needed the source pad before, but now we do, and this op only >>>> needs to be implemented if the default source pad != 0. >>> >>> I'm not too fond of that. Is there something wrong with the method >>> implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER >>> an issue ? >> >> 1) it's a heck of a lot of code just to get a simple source pad that the >> subdev knows anyway, > > I don't think the subdev knows. If a subdev has multiple source pads there's > no concept of a default source. It all depends on how the subdevs are > connected, and media_entity_remote_pad() is the right way to find out. Note that with 'source pad' I am referring to the output pad of a subdev (ADV7604_PAD_SOURCE in the case of the adv7604). There may be some confusion here. In my experience subdevs in a capture path have usually multiple input (sink) pads, but only one output (source) pad. Subdevs in a video output path tend to have one input (sink) pad and multiple output (source) pads. The multiple inputs/outputs are things like composite, S-Video, HDMI, VGA, etc. and the single input/output pad is where the device is hooked up to the mediabus which in turn connects to a DMA engine. >> 2) soc-camera doesn't use the media controller today, so this would add a >> dependency on the mc just for this, > > I agree that we shouldn't pull the whole MC userspace API in just for this, > but the kernel side of the API should be available as pad-level operations > depend on MC. We could split the CONFIG_MEDIA_CONTROLLER option in two. The way it is now is pretty OK. We just miss the information about the pad that feeds the dma capture path and for output we miss the the pad that is fed by the dma output path. Bridge drivers currently just assume pad 0 in all cases, but that's obviously not always right as the adv7604 illustrates. The alternative would be to just hardcode it in platform data or card information. What this patch does is just enumerating pads until it finds the first one that fits the criteria, which fails as well if there are multiple pads that would match and is a lot more code than just have the subdev provide the information. Regards, Hans >> 3) it doesn't actually make a media device, it is just fakes enough to make >> the subdev think it there is an MC. >> >> It doesn't actually have to be a new op, it could be a new field in >> v4l2_subdev as well. For those bridge drivers that do not use the MC but do >> need to know the source pad this is important information. >> >> It might even simplify the device tree if the default source pad is implied >> unless stated otherwise (but that might be a step too far). >> >> I wonder if a default input pad might also be useful to expose. I suspect >> this might become important if we want to add MC support to all existing >> v4l2 drivers. > > The concept of a default sink pad makes more sense, but I'm not sure I like it > too much either. I'd have to think about it. > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Monday 25 May 2015 00:43:25 Hans Verkuil wrote: > On 05/24/2015 11:50 PM, Laurent Pinchart wrote: > > On Sunday 24 May 2015 10:08:15 Hans Verkuil wrote: > >> On 05/23/2015 08:32 PM, Laurent Pinchart wrote: > >>> On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote: > >>>> On 05/20/2015 06:39 PM, William Towle wrote: > >>>>> Add detection of source pad number for drivers aware of the media > >>>>> controller API, so that soc_camera/rcar_vin can create device nodes > >>>>> to support a driver such as adv7604.c (for HDMI on Lager) underneath. > >>>>> > >>>>> Signed-off-by: William Towle <william.towle@codethink.co.uk> > >>>>> Reviewed-by: Rob Taylor <rob.taylor@codethink.co.uk> > >>>>> --- > >>>>> > >>>>> drivers/media/platform/soc_camera/rcar_vin.c | 4 ++++ > >>>>> drivers/media/platform/soc_camera/soc_camera.c | 27 ++++++++++++++- > >>>>> include/media/soc_camera.h | 1 + > >>>>> 3 files changed, 31 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c > >>>>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43 > >>>>> 100644 > >>>>> --- a/drivers/media/platform/soc_camera/rcar_vin.c > >>>>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c > >>>>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct > >>>>> soc_camera_device *icd, unsigned int idx, > >>>>> struct v4l2_mbus_framefmt *mf = &fmt.format; > >>>>> struct v4l2_rect rect; > >>>>> struct device *dev = icd->parent; > >>>>> + struct media_pad *remote_pad; > >>>>> int shift; > >>>>> > >>>>> + remote_pad = media_entity_remote_pad( > >>>>> + &icd->vdev->entity.pads[0]); > >>>>> + fmt.pad = remote_pad->index; > >>>> > >>>> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these > >>>> media calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER. > >>>> > >>>> Unfortunately, if it is not defined, then you still have no way of > >>>> finding the source pad. > >>>> > >>>> Laurent, do you think if it would make sense to add a new subdev core > >>>> op that will return the default source pad (I'm saying 'default' in > >>>> case there are more) of a subdev? That way it can be used in non-MC > >>>> drivers. We never needed the source pad before, but now we do, and this > >>>> op only needs to be implemented if the default source pad != 0. > >>> > >>> I'm not too fond of that. Is there something wrong with the method > >>> implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER > >>> an issue ? > >> > >> 1) it's a heck of a lot of code just to get a simple source pad that the > >> subdev knows anyway, > > > > I don't think the subdev knows. If a subdev has multiple source pads > > there's no concept of a default source. It all depends on how the subdevs > > are connected, and media_entity_remote_pad() is the right way to find > > out. > > Note that with 'source pad' I am referring to the output pad of a subdev > (ADV7604_PAD_SOURCE in the case of the adv7604). There may be some confusion > here. That's what I had understood :-) > In my experience subdevs in a capture path have usually multiple input > (sink) pads, but only one output (source) pad. Subdevs in a video output > path tend to have one input (sink) pad and multiple output (source) pads. > > The multiple inputs/outputs are things like composite, S-Video, HDMI, VGA, > etc. and the single input/output pad is where the device is hooked up to > the mediabus which in turn connects to a DMA engine. There's still no concept of a "default source" in the general case, but I assume you want to target the most common case where the subdev will have a single source pad ? A new operation (or field) could possibly work in some cases, but I feel it's a hack that lacks genericity. The above change is pretty simple from a bridge driver point of view, it's a single function call that will return the index of the connected pad. Sure, there's a little bit more core behind it compared to reading a fixed value, but it doesn't seem much to me, especially given that, in most of the cases, there will be a single link to check. > >> 2) soc-camera doesn't use the media controller today, so this would add a > >> dependency on the mc just for this, > > > > I agree that we shouldn't pull the whole MC userspace API in just for > > this, but the kernel side of the API should be available as pad-level > > operations depend on MC. We could split the CONFIG_MEDIA_CONTROLLER option > > in two. > > The way it is now is pretty OK. We just miss the information about the pad > that feeds the dma capture path and for output we miss the the pad that is > fed by the dma output path. > > Bridge drivers currently just assume pad 0 in all cases, but that's > obviously not always right as the adv7604 illustrates. It's indeed a bad assumption. Drivers that are MC-aware use the correct pad numbers. For drivers that are getting ported to the pad-level API without being MC-aware it's tempting to just hardcode the pad number to 0 as that's a common case when dealing with sensors, but that's plain wrong. > The alternative would be to just hardcode it in platform data or card > information. What this patch does is just enumerating pads until it finds > the first one that fits the criteria, which fails as well if there are > multiple pads that would match and is a lot more code than just have the > subdev provide the information. media_entity_remote_source() doesn't enumerate pads, it loops over links, so as long as links are set up properly there should be no issue, even if the remote subdev has multiple source pads. That's why I believe media_entity_remote_source() is the way to go. > >> 3) it doesn't actually make a media device, it is just fakes enough to > >> make the subdev think it there is an MC. > >> > >> It doesn't actually have to be a new op, it could be a new field in > >> v4l2_subdev as well. For those bridge drivers that do not use the MC but > >> do need to know the source pad this is important information. > >> > >> It might even simplify the device tree if the default source pad is > >> implied unless stated otherwise (but that might be a step too far). > >> > >> I wonder if a default input pad might also be useful to expose. I suspect > >> this might become important if we want to add MC support to all existing > >> v4l2 drivers. > > > > The concept of a default sink pad makes more sense, but I'm not sure I > > like it too much either. I'd have to think about it.
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43 100644 --- a/drivers/media/platform/soc_camera/rcar_vin.c +++ b/drivers/media/platform/soc_camera/rcar_vin.c @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx, struct v4l2_mbus_framefmt *mf = &fmt.format; struct v4l2_rect rect; struct device *dev = icd->parent; + struct media_pad *remote_pad; int shift; + remote_pad = media_entity_remote_pad( + &icd->vdev->entity.pads[0]); + fmt.pad = remote_pad->index; ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt); if (ret < 0) return ret; diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c index d708df4..126d645 100644 --- a/drivers/media/platform/soc_camera/soc_camera.c +++ b/drivers/media/platform/soc_camera/soc_camera.c @@ -1293,6 +1293,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd) .which = V4L2_SUBDEV_FORMAT_ACTIVE, }; struct v4l2_mbus_framefmt *mf = &fmt.format; + int src_pad_idx = -1; int ret; sd->grp_id = soc_camera_grp_id(icd); @@ -1311,7 +1312,25 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd) } /* At this point client .probe() should have run already */ - ret = soc_camera_init_user_formats(icd); + ret = media_entity_init(&icd->vdev->entity, 1, &icd->pad, 0); + if (!ret) { + for (src_pad_idx = 0; src_pad_idx < sd->entity.num_pads; + src_pad_idx++) + if (sd->entity.pads[src_pad_idx].flags + == MEDIA_PAD_FL_SOURCE) + break; + + if (src_pad_idx < sd->entity.num_pads) { + if (!media_entity_create_link( + &icd->vdev->entity, 0, + &sd->entity, src_pad_idx, + MEDIA_LNK_FL_IMMUTABLE | + MEDIA_LNK_FL_ENABLED)) { + ret = soc_camera_init_user_formats(icd); + } + } + } + if (ret < 0) goto eusrfmt; @@ -1322,6 +1341,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd) goto evidstart; /* Try to improve our guess of a reasonable window format */ + fmt.pad = src_pad_idx; if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) { icd->user_width = mf->width; icd->user_height = mf->height; @@ -1335,6 +1355,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd) evidstart: soc_camera_free_user_formats(icd); eusrfmt: + media_entity_cleanup(&icd->vdev->entity); soc_camera_remove_device(icd); return ret; @@ -1856,6 +1877,10 @@ static int soc_camera_remove(struct soc_camera_device *icd) if (icd->num_user_formats) soc_camera_free_user_formats(icd); + if (icd->vdev->entity.num_pads) { + media_entity_cleanup(&icd->vdev->entity); + } + if (icd->clk) { /* For the synchronous case */ v4l2_clk_unregister(icd->clk); diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h index 2f6261f..f0c5238 100644 --- a/include/media/soc_camera.h +++ b/include/media/soc_camera.h @@ -42,6 +42,7 @@ struct soc_camera_device { unsigned char devnum; /* Device number per host */ struct soc_camera_sense *sense; /* See comment in struct definition */ struct video_device *vdev; + struct media_pad pad; struct v4l2_ctrl_handler ctrl_handler; const struct soc_camera_format_xlate *current_fmt; struct soc_camera_format_xlate *user_formats;