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

[Maxime Ripard]

On Wed, May 31, 2017 at 09:11:15PM -0600, Simon Glass wrote:
> 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.

That was my first suggestion, so I'm fine with that :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170601/ec58bb59/attachment.sig>