From: Steve Longerbeam <slongerbeam@gmail.com>
To: Russell King - ARM Linux <linux@armlinux.org.uk>,
Philipp Zabel <p.zabel@pengutronix.de>
Cc: robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org,
kernel@pengutronix.de, fabio.estevam@nxp.com, mchehab@kernel.org,
hverkuil@xs4all.nl, nick@shmanahar.org,
markus.heiser@darmarIT.de,
laurent.pinchart+renesas@ideasonboard.com, bparrot@ti.com,
geert@linux-m68k.org, arnd@arndb.de, sudipm.mukherjee@gmail.com,
minghsiu.tsai@mediatek.com, tiffany.lin@mediatek.com,
jean-christophe.trotin@st.com, horms+renesas@verge.net.au,
niklas.soderlund+renesas@ragnatech.se, robert.jarzmik@free.fr,
songjun.wu@microchip.com, andrew-ct.chen@mediatek.com,
gregkh@linuxfoundation.org, shuah@kernel.org,
sakari.ailus@linux.intel.com, pavel@ucw.cz,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-media@vger.kernel.org, devel@driverdev.osuosl.org,
Steve Longerbeam <steve_longerbeam@mentor.com>
Subject: Re: [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1
Date: Tue, 4 Apr 2017 17:44:05 -0700 [thread overview]
Message-ID: <7235285c-f39a-64bc-195a-11cfde9e67c5@gmail.com> (raw)
In-Reply-To: <19f0ce92-cad6-8950-8018-e3224e2bf266@gmail.com>
On 04/04/2017 05:40 PM, Steve Longerbeam wrote:
>
>
> On 04/04/2017 04:10 PM, Russell King - ARM Linux wrote:
>> On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
>>> The TVP5150 DT bindings specify a single output port (port 0) that
>>> corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>> I'm trying to get this to work with a TVP5150 analog TV decoder, and the
>>> first problem is that this device doesn't have pad 0 as its single
>>> output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
>>> pads (input, video out, vbi out, audio out), and video out is pad 1,
>>> whereas the device tree only defines a single port (0).
>>
>> Looking at the patch, it's highlighted another review point with
>> Steve's driver.
>>
>>> diff --git a/drivers/staging/media/imx/imx-media-dev.c
>>> b/drivers/staging/media/imx/imx-media-dev.c
>>> index 17e2386a3ca3a..c52d6ca797965 100644
>>> --- a/drivers/staging/media/imx/imx-media-dev.c
>>> +++ b/drivers/staging/media/imx/imx-media-dev.c
>>> @@ -267,6 +267,15 @@ static int imx_media_create_link(struct
>>> imx_media_dev *imxmd,
>>> source_pad = link->local_pad;
>>> sink_pad = link->remote_pad;
>>>
>>> + /*
>>> + * If the source subdev is an analog video decoder with a single
>>> source
>>> + * port, assume that this port 0 corresponds to the
>>> DEMOD_PAD_VID_OUT
>>> + * entity pad.
>>> + */
>>> + if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
>>> + local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
>>> + source_pad = DEMOD_PAD_VID_OUT;
>>> +
>>> v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
>>> source->name, source_pad, sink->name, sink_pad);
>>
>> What is "local" and what is "remote" here? It seems that, in the case of
>> a link being created with the sensor(etc), it's all back to front.
>>
>> Eg, I see locally:
>>
>> imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0
>>
>> So here, "source" is the imx219 (the sensor), and sink is
>> "imx6-mipi-csi2"
>> (part of the iMX6 capture.) However, this makes "local_sd" the subdev of
>> the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
>> totally back to front - this code is part of the iMX6 capture system,
>> so "local" implies that it should be part of that, and the "remote" thing
>> would be the sensor.
>>
>> Hence, this seems completely confused. I'd suggest that:
>>
>> (a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in
>> imx_media_create_link()
>> is moved into imx_media_create_links(), and placed here instead:
>>
>> for (j = 0; j < num_pads; j++) {
>> pad = &local_sd->pad[j];
>>
>> if (pad->pad.flags & MEDIA_PAD_FL_SINK)
>> continue;
>>
>> ...
>> }
>>
>> as the pad isn't going to spontaneously change this flag while we
>> consider each individual link.
>
>
> Sure, I can do that. It would avoid iterating unnecessarily through the
> pad's links if the pad is a sink.
>
>
>> However, maybe the test should be:
>>
>> if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))
>>
>> ?
>>
>
> maybe that is more intuitive.
>
>
>> (b) the terms "local" and "remote" in imx_media_create_link() are
>> replaced with "source" and "sink" respectively, since this will
>> now be called with a guaranteed source pad.
>
> Agreed. I'll change the arg and local var names.
>
>>
>> As for Philipp's solution, I'm not sure what the correct solution for
>> something like this is. It depends how you view "hardware interface"
>> as defined by video-interfaces.txt, and whether the pads on the TVP5150
>> represent the hardware interfaces. If we take the view that the pads
>> do represent hardware interfaces, then using the reg= property on the
>> port node will solve this problem.
>
> And the missing port nodes would have to actually be defined first.
> According to Philipp they aren't, only a single output port 0.
>
>
>>
>> If not, it would mean that we would have to have the iMX capture code
>> scan the pads on the source device, looking for outputs - but that
>> runs into a problem - if the source device has multiple outputs, does
>> the reg= property specify the output pad index or the pad number,
>
> And how do we even know the data direction of a DT port. Is it an input,
> an output, bidirectional? The OF graph parsing in imx-media-of.c can't
> determine a port's direction if it encounters a device it doesn't
> recognize that has multiple ports. For now that is not really a problem
> because upstream from the video mux and csi-2 receiver it's expected
> there will only be original sources of video with only one source port.
> But it can become a limitation later. For example a device that has
> multiple output bus interfaces, which would require multiple output
> ports.
>
Actually what was I thinking, the TVP5150 is already an example of
such a device.
All of this could be solved if there was some direction information
in port nodes.
Steve
next prev parent reply other threads:[~2017-04-05 0:44 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-28 0:40 [PATCH v6 00/39] i.MX Media Driver Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 01/39] [media] dt-bindings: Add bindings for video-multiplexer device Steve Longerbeam
2017-04-03 14:14 ` Rob Herring
2017-03-28 0:40 ` [PATCH v6 02/39] [media] dt-bindings: Add bindings for i.MX media driver Steve Longerbeam
2017-03-29 0:21 ` Rob Herring
2017-03-29 0:35 ` Steve Longerbeam
2017-04-03 14:07 ` Rob Herring
2017-04-03 15:15 ` Russell King - ARM Linux
2017-03-29 8:39 ` Russell King - ARM Linux
2017-04-03 14:11 ` Rob Herring
2017-04-03 15:03 ` Russell King - ARM Linux
2017-03-28 0:40 ` [PATCH v6 03/39] [media] dt/bindings: Add bindings for OV5640 Steve Longerbeam
2017-04-03 14:15 ` Rob Herring
2017-03-28 0:40 ` [PATCH v6 04/39] ARM: dts: imx6qdl: Add compatible, clocks, irqs to MIPI CSI-2 node Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 05/39] ARM: dts: imx6qdl: Add mipi_ipu1/2 multiplexers, mipi_csi, and their connections Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 06/39] ARM: dts: imx6qdl: add capture-subsystem device Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 07/39] ARM: dts: imx6qdl-sabrelite: remove erratum ERR006687 workaround Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 08/39] ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 09/39] ARM: dts: imx6-sabresd: " Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 10/39] ARM: dts: imx6-sabreauto: create i2cmux for i2c3 Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 11/39] ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 12/39] ARM: dts: imx6-sabreauto: add pinctrl for gpt input capture Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 13/39] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 14/39] add mux and video interface bridge entity functions Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 15/39] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 16/39] [media] add Omnivision OV5640 sensor driver Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 17/39] platform: add video-multiplexer subdevice driver Steve Longerbeam
2017-03-28 14:12 ` Vladimir Zapolskiy
2017-04-04 12:47 ` Sakari Ailus
2017-04-12 0:50 ` Steve Longerbeam
2017-04-12 9:09 ` Sakari Ailus
2017-04-13 13:52 ` Philipp Zabel
2017-04-14 20:32 ` Pavel Machek
2017-04-18 8:09 ` Philipp Zabel
2017-04-18 9:05 ` Pavel Machek
2017-04-05 11:18 ` Pavel Machek
2017-04-05 11:58 ` Lucas Stach
2017-04-05 18:05 ` Pavel Machek
2017-03-28 0:40 ` [PATCH v6 18/39] media: Add userspace header file for i.MX Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 19/39] media: Add i.MX media core driver Steve Longerbeam
2017-03-30 17:25 ` [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1 Philipp Zabel
2017-04-04 22:11 ` Steve Longerbeam
2017-04-05 9:43 ` Philipp Zabel
2017-04-04 23:10 ` Russell King - ARM Linux
2017-04-05 0:40 ` Steve Longerbeam
2017-04-05 0:44 ` Steve Longerbeam [this message]
2017-04-05 8:21 ` Russell King - ARM Linux
2017-04-05 9:34 ` Philipp Zabel
2017-04-05 13:55 ` Javier Martinez Canillas
2017-04-05 14:53 ` Mauro Carvalho Chehab
2017-04-05 15:39 ` Devin Heitmueller
2017-04-05 16:17 ` Mauro Carvalho Chehab
2017-04-05 17:02 ` Devin Heitmueller
2017-04-05 17:16 ` Mauro Carvalho Chehab
2017-04-06 9:57 ` Philipp Zabel
2017-04-05 11:32 ` [PATCH v6 19/39] media: Add i.MX media core driver Pavel Machek
2017-04-05 11:34 ` Pavel Machek
2017-04-06 9:43 ` Philipp Zabel
2017-04-06 23:51 ` Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 20/39] media: imx: Add Capture Device Interface Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 21/39] media: imx: Add CSI subdev driver Steve Longerbeam
2017-04-06 13:55 ` [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default Philipp Zabel
2017-04-06 14:05 ` Russell King - ARM Linux
2017-04-06 15:01 ` Philipp Zabel
2017-04-06 15:10 ` Russell King - ARM Linux
2017-04-06 15:25 ` Philipp Zabel
2017-04-13 0:33 ` Steve Longerbeam
2017-04-13 0:45 ` [PATCH 40/40] media: imx: set and propagate empty field, colorimetry params Steve Longerbeam
2017-04-13 10:09 ` Philipp Zabel
2017-04-13 16:40 ` Steve Longerbeam
2017-04-18 9:30 ` Philipp Zabel
2017-05-08 9:41 ` Philipp Zabel
2017-05-09 3:44 ` Steve Longerbeam
2017-04-06 14:20 ` [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default Hans Verkuil
2017-04-06 14:38 ` Russell King - ARM Linux
2017-04-06 14:54 ` Philipp Zabel
2017-04-06 15:43 ` Hans Verkuil
2017-04-06 16:01 ` Philipp Zabel
2017-04-12 7:03 ` Hans Verkuil
2017-04-13 10:07 ` Philipp Zabel
2017-04-06 15:18 ` Philipp Zabel
2017-05-08 8:27 ` Hans Verkuil
2017-05-08 9:36 ` Philipp Zabel
2017-05-08 10:12 ` Hans Verkuil
2017-03-28 0:40 ` [PATCH v6 22/39] media: imx: Add VDIC subdev driver Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 23/39] media: imx: Add IC subdev drivers Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 24/39] media: imx: Add MIPI CSI-2 Receiver subdev driver Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 25/39] ARM: imx_v6_v7_defconfig: Enable staging video4linux drivers Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 26/39] media: imx: add support for bayer formats Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 27/39] media: imx: csi: " Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 28/39] media: imx: csi: fix crop rectangle changes in set_fmt Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 29/39] media: imx: csi: add __csi_get_fmt Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 30/39] media: imx: csi/fim: add support for frame intervals Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 31/39] media: imx: redo pixel format enumeration and negotiation Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 32/39] media: imx: csi: add frame skipping support Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 33/39] media: imx: csi: Avoid faulty sensor frames at stream start Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 34/39] media: imx: csi: fix crop rectangle reset in sink set_fmt Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 35/39] media: imx: propagate sink pad formats to source pads Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 36/39] media: imx: csi: add sink selection rectangles Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 37/39] media: imx-csi: add frame size/interval enumeration Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 38/39] media: imx-ic-prpencvf: add frame size enumeration Steve Longerbeam
2017-03-28 0:40 ` [PATCH v6 39/39] media: imx-media-capture: add frame sizes/interval enumeration Steve Longerbeam
2017-03-30 11:02 ` [PATCH v6 00/39] i.MX Media Driver Russell King - ARM Linux
2017-03-30 16:12 ` Steve Longerbeam
2017-03-30 16:27 ` Russell King - ARM Linux
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=7235285c-f39a-64bc-195a-11cfde9e67c5@gmail.com \
--to=slongerbeam@gmail.com \
--cc=andrew-ct.chen@mediatek.com \
--cc=arnd@arndb.de \
--cc=bparrot@ti.com \
--cc=devel@driverdev.osuosl.org \
--cc=devicetree@vger.kernel.org \
--cc=fabio.estevam@nxp.com \
--cc=geert@linux-m68k.org \
--cc=gregkh@linuxfoundation.org \
--cc=horms+renesas@verge.net.au \
--cc=hverkuil@xs4all.nl \
--cc=jean-christophe.trotin@st.com \
--cc=kernel@pengutronix.de \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mark.rutland@arm.com \
--cc=markus.heiser@darmarIT.de \
--cc=mchehab@kernel.org \
--cc=minghsiu.tsai@mediatek.com \
--cc=nick@shmanahar.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=p.zabel@pengutronix.de \
--cc=pavel@ucw.cz \
--cc=robert.jarzmik@free.fr \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=shawnguo@kernel.org \
--cc=shuah@kernel.org \
--cc=songjun.wu@microchip.com \
--cc=steve_longerbeam@mentor.com \
--cc=sudipm.mukherjee@gmail.com \
--cc=tiffany.lin@mediatek.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).