Re: Regression: tvp5150 refactoring breaks all em28xx devices

2016-12-08 Thread Mauro Carvalho Chehab 
Hi Laurent and Devin,

Em Thu, 08 Dec 2016 00:59:17 +0200
Laurent Pinchart  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 
> 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 
> 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 
>

Re: Regression: tvp5150 refactoring breaks all em28xx devices

2016-12-07 Thread Laurent Pinchart 
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 
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 
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 
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 

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

Regression: tvp5150 refactoring breaks all em28xx devices

2016-12-07 Thread Devin Heitmueller 
Hello Javier, Mauro, Laurent,

I hope all is well with you.  Mauro, Laurent:  you guys going to
ELC/Portland in February?

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.

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.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html