Hi Devin,
On Wednesday 07 Dec 2016 12:47:01 Devin Heitmueller wrote:
> Hello Javier, Mauro, Laurent,
>
> I hope all is well with you. Mauro, Laurent: you guys going to
> ELC/Portland in February?
I haven't decided for sure yet, but I will likely go.
> Looks like the refactoring done to tvp5150 in January 2016 for
> s_stream() to support some embedded platform caused breakage in the
> 30+ em28xx products that also use the chip.
I assume you're talking about
commit 460b6c0831cb52ef349156cfa27e889606b4cb75
Author: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Date: Thu Jan 7 10:46:45 2016 -0200
[media] tvp5150: Add s_stream subdev operation support
followed by
commit 47de9bf8931e6bf9c92fdba9867925d1ce482ab1
Author: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
Date: Mon Jan 25 14:39:34 2016 -0200
[media] tvp5150: Fix breakage for serial usage
both introduced in v4.6. I further assume that "serial" means BT.656 here,
which is still parallel.
> Problem confirmed on both the Startech SVIDUSB2 board Steve Preston
> was nice enough to ship me (after adding a board profile), as well as
> on my original HVR-950 which has worked fine since 2008.
>
> The implementation tramples the TVP5150_MISC_CTL register, blowing
> into it a hard-coded value based on one of two scenarios, neither of
> which matches what is expected by em28xx devices. At least in the
> case of NTSC, this results in chroma cycling. This was also reported
> by Alexandre-Xavier Labonté-Lamoureux back in August, although in the
> video below he's also having some other issue related to progressive
> video because he's using an old gaming console as the source (i.e. pay
> attention to the chroma effects in the top half of the video rather
> than the fact that only the first field is being rendered).
>
> https://youtu.be/WLlqJ7T3y4g
>
> The s_stream implementation writes 0x09 or 0x0d into TVP5150_MISC_CTL
> (overriding whatever was written by tvp5150_init_default and
> tvp5150_selmux(). In fact, just as a test I was able to start up
> video, see the corruption, and write the correct value back into the
> register via v4l2-dbg in order to get it working again:
>
> sudo v4l2-dbg --chip=subdev0 --set-register=0x03 0x6f
>
> There's no easy fix for this without extending the driver to support
> proper configuration of the output pin muxing, which it isn't clear to
> me what the right approach is and I don't have the embedded hardware
> platform that prompted the refactoring in order to do regression
> testing anyway.
>
> Feel free to take it upon yourselves to fix the regression you introduced.
I've had a quick look at the code from the point of view opposite from yours,
with my knowledge of the embedded side but without any experience with em28xx.
I don't think that adding proper configuration of pinmuxing would be that
hard, if it wasn't for the tvp5150_reset() function. The function is called
directly in the get and set format handlers, and through subdev core
operations.
The way we expose and use the reset operation is a very surprising (to stay
politically correct) idea, but in the context of em28xx shouldn't be too much
of a problem as the operation is only invoked at stream on time, before
s_stream(1). However, calling it from the get and set format handlers can only
lead me to conclude that the kernel is missing an ENOBRAIN error code. I'll
blame it on history.
As a prerequisite to implement proper output mixing configuration, the
tvp5150_reset() call needs to be removed from tvp5150_fill_fmt(). I can't test
that with the em28xx driver as I don't have access to any such device. Devin,
would you be able to assist with testing on em28xx by removing the function
call from a working kernel (v4.5 would be ideal) and check if the device still
operates correctly ? I believe it would, given that the reset operation is
called at stream on time as well as explained above, and that call would still
be there.
The tvp5150_reset() call in tvp5150_fill_fmt() was added by
commit ec2c4f3f93cb9ae2b09b8e942dd75ad3bdf23c9d
Author: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
Date: Thu Jan 5 10:57:39 2012 -0300
[media] media: tvp5150: Add mbus_fmt callbacks
These callbacks allow a host video driver
to poll video formats supported by tvp5150.
Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
I assume the call was originally intended to put the device in a known state
for a following call to tvp5150_read_std() in the same function. Given that
that code got removed in the meantime, I don't see any need to reset the chip
there.
I'm not sure who added the code, as the commit is authored by Javier by only
signed by Mauro. Could any (or both) of you shed some light on that ?
Mauro, as you've already attempted (unfortunately unsuccessfully) to fix this
problem in 47de9bf8931e6bf9c92fdba9867925d1ce482ab1, do you plan to give it
another try ? Now that I've performed an initial analysis and set the
direction, this should be easy, right ? :-)
--
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
