Re: [PATCH v1 05/35] drm/connector: Add TV standard property

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 6396 bytes --]

Hi,

On Fri, Aug 19, 2022 at 11:35:42AM +0200, Geert Uytterhoeven wrote:
> On Thu, Aug 18, 2022 at 5:34 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > On Thu, Aug 18, 2022 at 05:20:42PM +0200, Geert Uytterhoeven wrote:
> > > On Thu, Aug 18, 2022 at 4:54 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > On Wed, Aug 17, 2022 at 04:04:24PM +0200, Geert Uytterhoeven wrote:
> > > > > On Wed, Aug 17, 2022 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote:
> > > > > > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or
> > > > > > > > > > > > > > > > > handle them through "@<refresh>".  The latter would impact "[PATCH v1
> > > > > > > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as
> > > > > > > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the
> > > > > > > > > > > > > > > > parser, and we're going to use a named mode anyway.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a
> > > > > > > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main
> > > > > > > > > > > > > > > > > mode name, I think you want to add them here.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50,
> > > > > > > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p
> > > > > > > > > > > > >
> > > > > > > > > > > > > Above you said "I think the former would make more sense", so that
> > > > > > > > > > > > > should be "1280x720,tv_mode=hd720p50"?
> > > > > > > > > > > >
> > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p
> > > > > > > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p
> > > > > > > > > > >
> > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes.
> > > > > > > > > >
> > > > > > > > > > I agree, and I don't see how that command-line doesn't express that?
> > > > > > > > >
> > > > > > > > > Oh, I see what you mean: yes, it expresses that.
> > > > > > > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes,
> > > > > > > > > where the TV mode specifies both number of lines and frame rate.
> > > > > > > >
> > > > > > > > Only if we're using a named mode, and naming is hard :)
> > > > > > >
> > > > > > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with
> > > > > > > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a
> > > > > > > mode with 525 lines and 30 frames/s.
> > > > > >
> > > > > > In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid:
> > > > > >
> > > > > > https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/
> > > > >
> > > > > It would become supported once the ideas from thread "[PATCH v1 04/35]
> > > > > drm/modes: Introduce 480i and 576i modes" are implemented...
> > > >
> > > > Indeed, but I'm still not sure what your concern is here.
> > > > "640x480,tv_mode=PAL-N" and "640x480,tv_mode=PAL-M" are both fairly
> > > > obvious.
> > > >
> > > > You were initially saying that you had concern over the inconsistency of
> > > > NTSC/PAL/SECAM where the TV mode would specify a number of lines and
> > > > frame rate, but hd720p50 also specifies a number of line and frame rate?
> > >
> > > My concern is that you want to call the TV mode "hd720p", which
> > > does not dictate the frame rate.
> > > I would like to have both "720p50" and "720p60", as they do dictate
> > > the frame rate, like all the non-hd modes.
> >
> > But they don't?
> >
> > The refresh rate is part of the drm_display_mode, whereas that property
> > is metadata and entirely separate from the display mode.
> >
> > You can even change it without changing the mode at all
> 
> Yes, the refresh rate is part of drm_display_mode.  Vdisplay also
> is, but that doesn't mean you can set it to e.g. 700 when using
> "tv_mode=PAL-B". Some (combination of) parameters in drm_display_mode
> are dictated by the tv_mode.

But the opposite is also true: PAL-B and SECAM-B would be two different
TV mode, but (could) have the same display mode.

There's no equivalence or implication in that relationship, except for a
smaller set of those parameters. But it's the entire display mode that
we should compare.

> Perhaps the meaning of "tv_mode" should be clarified? What does it
> really mean, and what parameters does it (not) constrain?

As far as I'm concerned, it's only about the encoding. We can check
after the fact that, say, you don't try to use a mode line with than 525
lines and some NTSC variant, but the mode has precedence over the
property.

> For e.g. "PAL-B", I know it's a mode with 625 lines and 30 frames/s
> (60 fields/s).
> For "hd720p" I know it is an analog mode with 750 lines, but it's still
> ambiguous, as I don't know if it is the variant with 60 or 50 frames/s.

As far as the TV mode property is concerned, it doesn't encode neither
whether it has 750 lines, nor the refresh rate.

If you're talking about a named mode, then yeah, it's basically an alias
for a mode + a property, so it does, but we can choose names that aren't
ambiguous.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]