[U-Boot] [PATCH v2 4/4] sunxi: video: Add H3/H5 TV out driver

[Simon Glass]

Hi Maxime,

On 30 May 2017 at 14:41, Maxime Ripard <maxime.ripard at free-electrons.com> wrote:
> On Wed, May 24, 2017 at 05:34:52PM +0200, Jernej Škrabec wrote:
>> Hi,
>>
>> Dne torek, 23. maj 2017 ob 22:22:14 CEST je Maxime Ripard napisal(a):
>> > Hi Jernej,
>> >
>> > On Mon, May 22, 2017 at 08:49:57PM +0200, Jernej Škrabec wrote:
>> > > Dne ponedeljek, 22. maj 2017 ob 09:35:56 CEST je Maxime Ripard napisal(a):
>> > > > On Fri, May 19, 2017 at 05:41:17PM +0200, Jernej Skrabec wrote:
>> > > > > This commit adds support for TV (composite) output.
>> > > > >
>> > > > > Because there is no mechanism to select TV standard, PAL is
>> > > > > hardcoded.
>> > > >
>> > > > I'd rather use a consistent mechanism with the old driver (even if we
>> > > > only support PAL right now and reject any other option), and using
>> > > > composite-pal as monitor.
>> > >
>> > > I have few arguments against that:
>> > >
>> > > 1. Code for parsing that env variable is in videomodes.[c|h], which is
>> > > clearly a part of an older video framework (ctfb). I didn't want to
>> > > include any legacy code.
>> > >
>> > > 2. Even if this code is used for parsing, it would bring a lot of
>> > > confusion. For now, we can say that docs/README.video does not apply to
>> > > H3 and newer SoCs. If we implement this only partially, we would need to
>> > > describe in details which of each setting is honored with the new driver
>> > > and which not. Even then, a lot of users would skip that description and
>> > > complain anyway.
>> > The issue with this, and we've been bitten very hard on this one with
>> > the CHIP, is that you don't really have a clear majority on that
>> > one. If you support only PAL, half the world will be left out, and
>> > same thing with NTSC (for some reason, we never needed to support the
>> > less common ones like PAL-M or NTSC-J, but that just might be because
>> > it never really sold that well in those countries, I don't have any
>> > numbers on that).
>> >
>> > The point is, if you just hardcode PAL for now, you will have half
>> > your users complain, and then, when we will introduce NTSC support
>> > eventually, we'll have to introduce some mechanism to switch between
>> > the two, then we'll probably break the behaviour our users relied on
>> > before, making the other half of our users pissed.
>> >
>> > I'm not sure this is something we should just discard, or at least the
>> > second part. Having the selection mechanism in place, even if we don't
>> > support all the settings and just report an error in the logs in such
>> > a case address the latter issue.
>> >
>> > You'll also need to address how to setup the overscan, since this is
>> > really something you want to have very quick.
>>
>> Ok, I'm prepared to tackle this. Do you think it is worth to delay driver
>> merge?
>
> Not per se, but that should definitely be part of the same release, so
> if you can make it by then, then we can merge that right now, and
> merge the rest later. If you can't, then we'll have to postpone it a
> bit.
>
>> > > 3. If anything is done in this direction, I think that it is better
>> > > to extend DM video framework so other drivers would benefit from
>> > > that work too.
>> >
>> > That makes sense, but again, this is a pre-requisite for me. And it's
>> > not that hard to support the video modelines with a device model
>> > driver, Linux does it, and you have a string identifier at the
>> > beginning of it. It just has to be deterministic, but I don't think
>> > this is an issue with U-Boot's DM.
>>
>> Ok, so how do you think we should implement this? If only composite
>> modes are supported in the string, someone might try to set hdmi
>> monitor. Should we just ignore such settings and maybe print a
>> warning?
>
> I'm not sure it was your question, but I would just reject any
> improper configuration, and not display anything in such a case.
>
>> Also the question for Simon, how should I merge code for parsing
>> video string from drivers/video/videomodes.c to DM video in a way to
>> be useful to most drivers?
>>
>> I guess this is the first time to support analog video output in DM
>> video driver and there are some things missing such as a way to
>> select TV standard and define overscan.
>
> I think it can be done in the same way linux does it here:
> http://elixir.free-electrons.com/linux/latest/source/drivers/video/fbdev/core/fb_cmdline.c#L35
>
> You basically check first that there is a string matching what your
> driver expects, and then get the options.
>
> We could imagine having some extra function to parse that string and
> give back a structure filled with whatever was set in that command
> line. We could imagine having the common custom properties to be
> parsed at that same time.
>
>> Or alternatively, I could make just quick edit to sunxi_tve.c driver
>> and directly use old functions as they are. That way we could get
>> something working very quickly, but I don't like much such approach.
>
> That's another approach, I'm fine with it as a temporary measure, but
> I'm not sure how Simon feels about it.

Well how about a function that decodes the string into a struct? That
function could be called by any drivers that need it.

Regards,
Simon


>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com