Re: [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1

[Steve Longerbeam <slongerbeam@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