Hi Mauro,
On Thursday 08 Dec 2016 10:53:33 Mauro Carvalho Chehab wrote:
> Em Thu, 08 Dec 2016 14:25:38 +0200 Laurent Pinchart escreveu:
> > On Thursday 08 Dec 2016 06:55:58 Mauro Carvalho Chehab wrote:
> >> changeset 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
> >> support") added a logic that overrides TVP5150_CONF_SHARED_PIN setting,
> >> depending on the type of bus set via the .set_fmt() subdev callback.
> >>
> >> This is known to cause trobules on devices that don't use a V4L2
> >> subdev devnode, and a fix for it was made by changeset 47de9bf8931e
> >> ("[media] tvp5150: Fix breakage for serial usage"). Unfortunately,
> >> such fix doesn't consider the case of progressive video inputs,
> >> causing chroma decoding issues on such videos, as it overrides not
> >> only the type of video output, but also other unrelated bits.
> >>
> >> So, instead of trying to guess, let's detect if the device is set
> >> via a V4L2 subdev node or not. If not, just ignore the bogus logic.
> >>
> >> Fixes: 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
> >> support") Cc: Devin Heitmueller <dheitmueller@xxxxxxxxxxxxxx>
> >> Cc: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> >> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>,
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> >> ---
> >>
> >> Devin,
> >>
> >> I didn't test this patch. As I explained on my previous e-mail, my
> >> current test scenario for analog TV inputs is not ideal, as I lack
> >> progressive video and RF output testcases.
> >>
> >> Could you please test if this will fix for you?
> >>
> >> Laurent/Javier,
> >>
> >> With regards to OMAP3, it would be good to try to reproduce the issues
> >> Devin noticed on your hardware, testing with both progressive and
> >> interlaced sources and checking if the chroma is being decoded properly
> >> or not with a NTSC signal.
> >>
> >> drivers/media/i2c/tvp5150.c | 7 ++++++-
> >> drivers/media/platform/pxa_camera.c | 1 +
> >> drivers/media/platform/soc_camera/soc_mediabus.c | 1 +
> >> include/media/v4l2-mediabus.h | 3 +++
> >> 4 files changed, 11 insertions(+), 1 deletion(-)
> >
> > [snip]
> >
> >> diff --git a/include/media/v4l2-mediabus.h
> >> b/include/media/v4l2-mediabus.h
> >> index 34cc99e093ef..8af6b96d628b 100644
> >> --- a/include/media/v4l2-mediabus.h
> >> +++ b/include/media/v4l2-mediabus.h
> >> @@ -70,11 +70,14 @@
> >> * @V4L2_MBUS_BT656: parallel interface with embedded
> >> synchronisation, can
> >> * also be used for BT.1120
> >> * @V4L2_MBUS_CSI2: MIPI CSI-2 serial interface
> >> + * @V4L2_MBUS_UNKNOWN: used to indicate that the device is not
> >> controlled
> >> + * via a V4L2 subdev devnode interface
> >
> > Please, don't. v4l2_mbus_type has nothing to do with subdev device nodes.
> > It identifies the type of physical bus used to carry video data.
>
> As explained, for the devices that the regression was introduced,
> there's no way to control the physical bus, as it is hardwired.
>
> For those devices, mbus_type is just an useless information.
But the fact that a field is not used in some cases is no excuse to abuse it
for something completely unrelated to its purpose.
> The problem that your patch introduced is that it now assumes that
> the mbus would be controlled via a V4L2 subdevice. This is a wrong
> assumption, and caused a regression.
>
> So, we need a simple way to get rid of the broken code on those
> devices, and that's the simplest way.
We need a correct way to fix the problem, and this isn't the correct way.
> If you didn't like the name, we could, instead, call it as:
> V4L2_MBUS_CONTROLLED_BY_VIDEODEV, with actually tells more about it.
No, V4L2_MBUS_* has *nothing* to do with how the device is controlled.
> ---
>
> That's said, as OMAP3 support was added considering this code,
> if the progressive video bug is also present there, we could
> try some other solution, although, in such case, it is not a
> regression. Unfortunately, I don't have any OMAP3 hardware with
> tvp5151 to test.
I do, so I could test that.
> So, if nobody comes with another solution that would work for
> both cases, I'll likely send this regression fix to 4.10, as
> it is better than reverting the broken patch, as reverting it
> would cause regressions for OMAP3.
You're modifying the in-kernel V4L2 API as a hack to work around a bug. Let's
not do that.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
