Hi Laurent and Devin, Em Thu, 08 Dec 2016 00:59:17 +0200 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> escreveu: > 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. I haven't decided yet, but probably I won't. > > 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. When I wrote my patch on the top of Laurent's one, I tested it, with both analog TV and composite input, and didn't notice any issue. I used a WinTV USB2 and HVR-950. I didn't test with progressive video input though, as I'm using a PVR-350 TV out to generate signals, with, AFIKT, generates only interlaced video. Unfortunately, analog TV signal broadcast ended last month, and I don't have any analog TV RF generator. I might still have an old VCR with a RF output, but need to check if it is on my garage. > > 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(). That shouldn't affect anything. The .set_fmt() callback is only called by drivers/media/v4l2-core/v4l2-subdev.c. As those devices don't use subdevs, removing tvp5150_reset() from tvp5150_fill_fmt() shouldn't affect anything. > 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 ? Such patch is authored by Javier. He probably forgot to sign it. Javier should explain it more, but I guess it is meant to switch between NTSC and PAL. > > 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 ? :-) The way it is, it worked on my past test scenarios. As I explained before, testing right now is more complex, as I lack progressive video output and analog TV RF output. I could try to setup an environment to test it, but it could take some time to find the needed gadgets. 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
