Mateusz Kwiatkowski
2022-Sep-01 22:00 UTC
[Nouveau] [PATCH v2 09/41] drm/connector: Add TV standard property
Hi Maxime, W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:> The TV mode property has been around for a while now to select and get the > current TV mode output on an analog TV connector. > > Despite that property name being generic, its content isn't and has been > driver-specific which makes it hard to build any generic behaviour on top > of it, both in kernel and user-space. > > Let's create a new bitmask tv norm property, that can contain any of the > analog TV standards currently supported by kernel drivers. Each driver can > then pass in a bitmask of the modes it supports.This is not a bitmask property anymore, you've just changed it to an enum. The commit message is now misleading.> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > +?? ?{ DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" }, > +?? ?{ DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" }, > +?? ?{ DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_60, "PAL-60" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_B, "PAL-B" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_D, "PAL-D" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_G, "PAL-G" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_H, "PAL-H" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_I, "PAL-I" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" }, > +?? ?{ DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" }, > +?? ?{ DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" }, > +?? ?{ DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" }, > +?? ?{ DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" }, > +?? ?{ DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" }, > +?? ?{ DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" }, > +?? ?{ DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" }, > +?? ?{ DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" }, > +};I did not comment on it the last time, but this list looks a little bit random. Compared to the standards defined by V4L2, you also define SECAM-60 (a good thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K, SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L, see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC). Like I mentioned previously, I'm personally not a fan of including all those CCIR/ITU system variants, as they don't mean any difference to the output unless there is an RF modulator involved. But I get it that they have already been used and regressing probably wouldn't be a very good idea. But in that case keeping it consistent with the set of values used by V4L2 would be wise, I think.> +/** > + * drm_mode_create_tv_properties - create TV specific connector properties > + * @dev: DRM device > + * @supported_tv_modes: Bitmask of TV modes supported (See DRM_MODE_TV_MODE_*) > + > + * Called by a driver's TV initialization routine, this function creates > + * the TV specific connector properties for a given device.? Caller is > + * responsible for allocating a list of format names and passing them to > + * this routine. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_mode_create_tv_properties(struct drm_device *dev, > +?? ??? ??? ??? ?? unsigned int supported_tv_modes)supported_tv_modes is supposed to be a bitmask of BIT(DRM_MODE_TV_MODE_*) (or (1<<DRM_MODE_TV_MODE_*)) rather than DRM_MODE_TV_MODE_* directly, but this is not said explicitly anywhere in this doc comment.> +?? ?/** > +?? ? * @DRM_MODE_TV_MODE_PAL_NC: Seems equivalent to > +?? ? * @DRM_MODE_TV_MODE_PAL_N. > +?? ? */ > +?? ?DRM_MODE_TV_MODE_PAL_NC,AFAIK, the entire reason that "PAL-Nc" is ever mentioned as something separate from PAL-N is a result of a misunderstanding or misreading of the CCIR/ITU documents. See also the posting signed as Alchaemist here: https://en.wikipedia.org/wiki/Talk:PAL#PAL-N_versus_PAL-Nc That being said, we probably want to keep it if we want to remaing compatible with the loads of software and drivers which enumerate those as separate systems. But from a technical standpoint, PAL-N and PAL-Nc (and N/PAL, PAL-CN etc.) are just different "spellings" referring to exactly the same system.> +?? ?/** > +?? ? * @DRM_MODE_TV_MODE_SECAM_K: CCIR System G together with the > +?? ? * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G but > +?? ? * with different channels. > +?? ? */ > +?? ?DRM_MODE_TV_MODE_SECAM_K, > + > +?? ?/** > +?? ? * @DRM_MODE_TV_MODE_SECAM_K1: CCIR System G together with the > +?? ? * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G and > +?? ? * @DRM_MODE_TV_MODE_SECAM_K but with different channels. > +?? ? */ > +?? ?DRM_MODE_TV_MODE_SECAM_K1,Typos: you meant CCIR Systems K and K1, not System G. Best regards, Mateusz Kwiatkowski
Geert Uytterhoeven
2022-Sep-02 07:35 UTC
[Nouveau] [PATCH v2 09/41] drm/connector: Add TV standard property
Hi Mateusz, On Fri, Sep 2, 2022 at 12:00 AM Mateusz Kwiatkowski <kfyatek at gmail.com> wrote:> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze: > > The TV mode property has been around for a while now to select and get the > > current TV mode output on an analog TV connector. > > > > Despite that property name being generic, its content isn't and has been > > driver-specific which makes it hard to build any generic behaviour on top > > of it, both in kernel and user-space. > > > > Let's create a new bitmask tv norm property, that can contain any of the > > analog TV standards currently supported by kernel drivers. Each driver can > > then pass in a bitmask of the modes it supports. > > This is not a bitmask property anymore, you've just changed it to an enum. > The commit message is now misleading. > > > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > > + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" }, > > + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" }, > > + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" }, > > + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" }, > > + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" }, > > + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" }, > > + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" }, > > + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" }, > > + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" }, > > + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" }, > > + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" }, > > + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" }, > > + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" }, > > + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" }, > > + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" }, > > + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" }, > > + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" }, > > + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" }, > > + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" }, > > +}; > > I did not comment on it the last time, but this list looks a little bit random. > > Compared to the standards defined by V4L2, you also define SECAM-60 (a good > thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K, > SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L, > see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC). > > Like I mentioned previously, I'm personally not a fan of including all those > CCIR/ITU system variants, as they don't mean any difference to the output unless > there is an RF modulator involved. But I get it that they have already been used > and regressing probably wouldn't be a very good idea. But in that case keeping > it consistent with the set of values used by V4L2 would be wise, I think.Exactly. Anything outputting RGB (e.g. through a SCART or VGA connector) doesn't care about the color subcarrier or modulator parts. Likewise, anything outputting CVBS doesn't care about the modulator part. Perhaps "generic" variants of NSTC and PAL/SECAM should be added, which would really just mean 525/60 resp. 625/50. Alternatively, the tv_mode field could be split in two parts (either two separate fields, or bitwise), to maintain a clear separation between lines/fields versus color encoding and RF modulation (with zero for the latter meaning a generic version)? That would also keep the door open for TV_MODE_405_50, TV_MODE_819_50, TV_MODE_750_50, TV_MODE_750_60, ... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Maxime Ripard
2022-Sep-07 12:10 UTC
[Nouveau] [PATCH v2 09/41] drm/connector: Add TV standard property
Hi, On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze: > > The TV mode property has been around for a while now to select and get the > > current TV mode output on an analog TV connector. > > > > Despite that property name being generic, its content isn't and has been > > driver-specific which makes it hard to build any generic behaviour on top > > of it, both in kernel and user-space. > > > > Let's create a new bitmask tv norm property, that can contain any of the > > analog TV standards currently supported by kernel drivers. Each driver can > > then pass in a bitmask of the modes it supports. > > This is not a bitmask property anymore, you've just changed it to an enum. > The commit message is now misleading. > > > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > > +?? ?{ DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" }, > > +?? ?{ DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" }, > > +?? ?{ DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_60, "PAL-60" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_B, "PAL-B" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_D, "PAL-D" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_G, "PAL-G" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_H, "PAL-H" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_I, "PAL-I" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" }, > > +?? ?{ DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" }, > > +?? ?{ DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" }, > > +?? ?{ DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" }, > > +?? ?{ DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" }, > > +?? ?{ DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" }, > > +?? ?{ DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" }, > > +?? ?{ DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" }, > > +?? ?{ DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" }, > > +}; > > I did not comment on it the last time, but this list looks a little bit random. > > Compared to the standards defined by V4L2, you also define SECAM-60 (a good > thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K, > SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L, > see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC). > > Like I mentioned previously, I'm personally not a fan of including all those > CCIR/ITU system variants, as they don't mean any difference to the output unless > there is an RF modulator involved. But I get it that they have already been used > and regressing probably wouldn't be a very good idea. But in that case keeping > it consistent with the set of values used by V4L2 would be wise, I think.Ack. What would be the list of standards we'd absolutely need? NSTC-M, NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?> > +/** > > + * drm_mode_create_tv_properties - create TV specific connector properties > > + * @dev: DRM device > > + * @supported_tv_modes: Bitmask of TV modes supported (See DRM_MODE_TV_MODE_*) > > + > > + * Called by a driver's TV initialization routine, this function creates > > + * the TV specific connector properties for a given device.? Caller is > > + * responsible for allocating a list of format names and passing them to > > + * this routine. > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_mode_create_tv_properties(struct drm_device *dev, > > +?? ??? ??? ??? ?? unsigned int supported_tv_modes) > > supported_tv_modes is supposed to be a bitmask of BIT(DRM_MODE_TV_MODE_*) > (or (1<<DRM_MODE_TV_MODE_*)) rather than DRM_MODE_TV_MODE_* directly, but this > is not said explicitly anywhere in this doc comment.The argument doc mentions that it's a "Bitmask of TV modes supported (See DRM_MODE_TV_MODE_*)", how would you improve it? Thanks! Maxime -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20220907/5bc5b438/attachment.sig>