Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Javier,
Hi Mauro,

On 18-07-31 13:26, Javier Martinez Canillas wrote:
> Hi Mauro,
> 
> On 07/31/2018 12:06 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 31 Jul 2018 10:52:56 +0200
> > Javier Martinez Canillas <javierm@redhat.com> escreveu:
> > 
> >> Hello Mauro,
> >>
> >> On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
> >>> Em Thu, 28 Jun 2018 18:20:50 +0200
> >>> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> >>>   
> >>>> From: Javier Martinez Canillas <javierm@redhat.com>
> >>>>
> >>>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> >>>> added input signals support for the tvp5150, but the approach was found
> >>>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> >>>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
> >>>>
> >>>> This left the driver with an undocumented (and wrong) DT parsing logic,
> >>>> so lets get rid of this code as well until the input connectors support
> >>>> is implemented properly.
> >>>>
> >>>> It's a partial revert due other patches added on top of mentioned commit
> >>>> not allowing the commit to be reverted cleanly anymore. But all the code
> >>>> related to the DT parsing logic and input entities creation are removed.
> >>>>
> >>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >>>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> [m.felsch@pengutronix.de: rm TVP5150_INPUT_NUM define]
> >>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> >>>> ---  
> >>>
> >>> ...
> >>>   
> >>>> -static int tvp5150_registered(struct v4l2_subdev *sd)
> >>>> -{
> >>>> -#ifdef CONFIG_MEDIA_CONTROLLER
> >>>> -	struct tvp5150 *decoder = to_tvp5150(sd);
> >>>> -	int ret = 0;
> >>>> -	int i;
> >>>> -
> >>>> -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> >>>> -		struct media_entity *input = &decoder->input_ent[i];
> >>>> -		struct media_pad *pad = &decoder->input_pad[i];
> >>>> -
> >>>> -		if (!input->name)
> >>>> -			continue;
> >>>> -
> >>>> -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> >>>> -
> >>>> -		ret = media_entity_pads_init(input, 1, pad);
> >>>> -		if (ret < 0)
> >>>> -			return ret;
> >>>> -
> >>>> -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> >>>> -		if (ret < 0)
> >>>> -			return ret;
> >>>> -
> >>>> -		ret = media_create_pad_link(input, 0, &sd->entity,
> >>>> -					    DEMOD_PAD_IF_INPUT, 0);
> >>>> -		if (ret < 0) {
> >>>> -			media_device_unregister_entity(input);
> >>>> -			return ret;
> >>>> -		}
> >>>> -	}
> >>>> -#endif  
> >>>
> >>> Hmm... I suspect that reverting this part may cause problems for drivers
> >>> like em28xx when compiled with MC, as they rely that the supported demods
> >>> will have 3 pads (DEMOD_NUM_PADS).
> >>>  
> >>
> >> I don't see how this change could affect em28xx and other drivers. The function
> >> tvp5150_registered() being removed here, only register the media entity and add
> >> a link if input->name was set. This is set in tvp5150_parse_dt() and only if a
> >> input connector as defined in the Device Tree file.
> >>
> >> In other words, all the code removed by this patch is DT-only and isn't used by
> >> any media driver that makes use of the tvp5151.
> >>
> >> As mentioned in the commit message, this code has never been used (besides from
> >> my testings) and should had been removed when the DT binding was reverted, but
> >> for some reasons the first patch landed and the second didn't at the time.
> > 
> > Short answer: 
> > 
> > Yeah, you're right. Yet, patch 19/22 will cause regressions.
> >
> > Long answer:
> > 
> > That's easy enough to test.
> > 
> > Without this patch, a em28xx-based board (Terratec Grabster AV350) reports:
> > 
> > $ ./mc_nextgen_test -D
> > digraph board {
> > 	rankdir=TB
> > 	colorscheme=x11
> > 	labelloc="t"
> > 	label="Grabster AV 350
> >  driver:em28xx, bus: usb-0000:00:14.0-2
> > "
> > 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> > 	entity_1 [label="{{<pad_2> 0} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_3> 1 | <pad_4> 2}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> > 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> > 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> > 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> > 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> > 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> > }
> > 
> > tvp5150 reports 3 pads (one input, two output pads), and media core
> > properly connects the source pads.
> > 
> > With patch 18/22, I got the same graph. So, yeah, applying this patch
> > won't cause regressions.
> >
> 
> Yes, I didn't have time to review the other patches in the set yet. I was just
> referring to patch 18/22 that it is really a standalone change and I've posted
> it several times already. So I think that one is safe to merge.
> 
> > However, when we apply patch 19/22:
> > 
> > $ mc_nextgen_test -D
> > digraph board {
> > 	rankdir=TB
> > 	colorscheme=x11
> > 	labelloc="t"
> > 	label="Grabster AV 350
> >  driver:em28xx, bus: usb-0000:00:14.0-2
> > "
> > 	intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
> > 	intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
> > 	entity_1 [label="{{<pad_2> 0 | <pad_3> 1 | <pad_4> 2} | entity_1\nATV decoder\ntvp5150 0-005c | {<pad_5> 3}}", shape=Mrecord, style=filled, fillcolor=lightblue]
> > 	entity_6 [label="{{<pad_12> 0} | entity_6\nV4L I/O\n2-2:1.0 video}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_9 [label="{{<pad_13> 0} | entity_9\nVBI I/O\n2-2:1.0 vbi}", shape=Mrecord, style=filled, fillcolor=aquamarine]
> > 	entity_14 [label="{entity_14\nunknown entity type\nComposite | {<pad_15> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	entity_16 [label="{entity_16\nunknown entity type\nS-Video | {<pad_17> 0}}", shape=Mrecord, style=filled, fillcolor=cadetblue]
> > 	intf_devnode_7 -> entity_6 [dir="none" color="orange"]
> > 	intf_devnode_10 -> entity_9 [dir="none" color="orange"]
> > 	entity_1:pad_3 -> entity_6:pad_12 [color=blue]
> > 	entity_1:pad_4 -> entity_9:pad_13 [color=blue]
> > 	entity_14:pad_15 -> entity_1:pad_2 [color=blue]
> > 	entity_16:pad_17 -> entity_1:pad_2 [color=blue style="dashed"]
> > }
> > 
> > The graph is not built correct, as it is linking tvp5150's input pads as
> > if they were output ones.

Maybe I misunderstand the mc-pads. I tought the pads represents the
physical ports. So I mapped these two things togehter.

> > 
> > The problem is that now you need to teach drivers/media/v4l2-core/v4l2-mc.c
> > to do the proper wiring for tvp5150.
> > 
> > I suspect that fixing v4l2-mc for doing that is not hard, but it may
> > require changes at the other demods. Thankfully there aren't many
> > demod drivers, but such patch should be applied before patch 19/22.
> > 
> > In the specific case of demods that don't support sliced VBI (or
> > where sliced VBI is not coded), there should be just one source pad.
> > 
> > On demods with sliced VBI, there are actually two source pads,
> > although, for simplicity, maybe we could map them as just one.
> > 
> > If we map as just one source pad, it is probably easy to change the
> > code at v4l2-mc to do the right thing.
> > 
> > I'll do some tests here and try to code it.
> >
> 
> Yes, another thing that patch 19/22 should take into account is DTs that
> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
> 
> In other words, it should work both when input connectors are defined in
> the DT and when these are not defined and only an output port is defined.

Yes, it would be a approach to map the output port dynamicaly to the
highest port number. I tried to keep things easy by a static mapping.
Maybe a follow up patch can change this behaviour.

Anyway, input connectors aren't required. There must be at least one
port child node with a correct port-number in the DT.

Regards,
Marco

> > Thanks,
> > Mauro
> > 
> 
> Best regards,