Em Thu, 08 Dec 2016 15:02:28 +0200
Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> escreveu:
> 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.
Well then your patch is wrong, as it *does* control the device based
on the value of V4L2_MBUS_*.
Anyway, there's another way to handle it: check if the device is
controlled via DT, enabling the new (IMHO broken - see below) behavior only
if it has DT. That will fix the regression without affecting OMAP3.
Patch sent.
Btw, the current logic is IMHO crap, as it just override all bits of
mbus, even the ones unrelated to bt656 setup at register TVP5150_MISC_CTL.
It also doesn't consider the other parts of the driver that change
the init values for S-Video setup, and do different init depending
on the version of the chipset (tvp5150 uses a different init than
tvp5150am1).
It also overrides TVP5150_CONF_SHARED_PIN from its default without
any good reason.
The right thing seems to touch only on the bits that control the
output format, and read the setup for TVP5150_CONF_SHARED_PIN
from DT.
>
> > ---
> >
> > 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.
>
Thanks,
Mauro
--
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
