Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
Hi,
On Fri, Feb 16, 2024 at 06:48:55PM GMT, Dave Stevenson wrote:
> From: Nick Hollinghurst
>
> Add this as a value for enum_drm_connector_tv_mode, represented
> by the string "Mono", to generate video with no colour encoding
> or bursts. Define it to have no pedestal (since only NTSC-M calls
> for a pedestal).
>
> Change default mode creation to acommodate the new tv_mode value
> which comprises both 525-line and 625-line formats.
>
> Signed-off-by: Nick Hollinghurst
> Signed-off-by: Dave Stevenson
> ---
> drivers/gpu/drm/drm_connector.c| 7 +++
> drivers/gpu/drm/drm_modes.c| 5 -
> drivers/gpu/drm/drm_probe_helper.c | 5 +++--
> include/drm/drm_connector.h| 7 +++
> 4 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae..fe05d27f3404 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1005,6 +1005,7 @@ static const struct drm_prop_enum_list
> drm_tv_mode_enum_list[] = {
> { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> { DRM_MODE_TV_MODE_SECAM, "SECAM" },
> + { DRM_MODE_TV_MODE_MONOCHROME, "Mono" },
> };
> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_
We'll need to have kunit tests here for that value in:
* drm_cmdline_parser_test to make sure it's properly read from the
command line
* drm_connector_test to make sure the name is properly associated to its
enum value
* drm_modes_test to make sure the modes generated for that tv mode
actually returns 625 lines
Those would all have been additional patches, and the next patches need
a new version as well so please add them to your v2.
Maxime
signature.asc
Description: PGP signature
Re: (subset) [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Fri, 16 Feb 2024 18:48:55 +, Dave Stevenson wrote: > Add this as a value for enum_drm_connector_tv_mode, represented > by the string "Mono", to generate video with no colour encoding > or bursts. Define it to have no pedestal (since only NTSC-M calls > for a pedestal). > > Change default mode creation to acommodate the new tv_mode value > which comprises both 525-line and 625-line formats. > > [...] Applied to misc/kernel.git (drm-misc-next). Thanks! Maxime
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Fri, Jun 14, 2024 at 11:01:45AM +0200, Daniel Vetter wrote: > On Thu, May 23, 2024 at 04:51:25PM +0200, Maxime Ripard wrote: > > Hi, > > > > Reviving this thread because I'm not sure what the outcome was. > > > > On Thu, Feb 29, 2024 at 11:52:12AM GMT, Daniel Vetter wrote: > > > > The only thing I'm saying is that this breaks the usual DRM > > > > requirements. > > > > If, as a maintainer, you're fine with breaking the rules and have a good > > > > motivation to do so, that's fine by me. Rules are meant to be broken > > > > from > > > > time to time depending on the situation. But please don't pretend that > > > > modetest/xrandr is valid user-space to pass the rules. > > > > > > I think it bends it pretty badly, because people running native Xorg are > > > slowly going away, and the modetest hack does not clear the bar for "is it > > > a joke/test/demo hack" for me. > > > > > > I think some weston (or whatever compositor you like) config file support > > > to set a bunch of "really only way to configure is by hand" output > > > properties would clear the bar here for me. Because that is a feature I > > > already mentioned that xrandr _does_ have, and which modetest hackery very > > > much does not. > > > > The expectation (and general usage) for that property was that it was > > set by the kernel command line and then was forgotten about. Old TVs > > require one mode and that's it, so it doesn't make much sense to change > > it while the system is live, you just want the default to work. > > > > So it's not really a matter of "the user-space code should be open" > > here, there's no user-space code, and there will likely never be given > > that it's mostly used to deal with decades-old systems at this point. > > Yeah if this is being used just with the kernel cmdline, then I guess > that's somewhat ok-ish. And TVs are horrible, so "massage your kernel > cmdline" is comparitively not a horrible interface :-P > > Anyway, I guess this makes this an ack. To clarify, since people asked about this on irc and why it's not a terrible precedence. This is very much just for very, very dead old tech like analog TVs. If people try this escape hatch for hdmi or dp screens, the answer will be completely different :-) -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Thu, May 23, 2024 at 04:51:25PM +0200, Maxime Ripard wrote: > Hi, > > Reviving this thread because I'm not sure what the outcome was. > > On Thu, Feb 29, 2024 at 11:52:12AM GMT, Daniel Vetter wrote: > > > The only thing I'm saying is that this breaks the usual DRM requirements. > > > If, as a maintainer, you're fine with breaking the rules and have a good > > > motivation to do so, that's fine by me. Rules are meant to be broken from > > > time to time depending on the situation. But please don't pretend that > > > modetest/xrandr is valid user-space to pass the rules. > > > > I think it bends it pretty badly, because people running native Xorg are > > slowly going away, and the modetest hack does not clear the bar for "is it > > a joke/test/demo hack" for me. > > > > I think some weston (or whatever compositor you like) config file support > > to set a bunch of "really only way to configure is by hand" output > > properties would clear the bar here for me. Because that is a feature I > > already mentioned that xrandr _does_ have, and which modetest hackery very > > much does not. > > The expectation (and general usage) for that property was that it was > set by the kernel command line and then was forgotten about. Old TVs > require one mode and that's it, so it doesn't make much sense to change > it while the system is live, you just want the default to work. > > So it's not really a matter of "the user-space code should be open" > here, there's no user-space code, and there will likely never be given > that it's mostly used to deal with decades-old systems at this point. Yeah if this is being used just with the kernel cmdline, then I guess that's somewhat ok-ish. And TVs are horrible, so "massage your kernel cmdline" is comparitively not a horrible interface :-P Anyway, I guess this makes this an ack. -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Thursday, February 29th, 2024 at 11:52, Daniel Vetter wrote: > I think some weston (or whatever compositor you like) config file support > to set a bunch of "really only way to configure is by hand" output > properties would clear the bar here for me. Because that is a feature I > already mentioned that xrandr does have, and which modetest hackery very > much does not. FWIW, this is a feature I would very much not want to add in my own compositor. Not only there is a bunch of complexity exposing KMS props in config files (enums, bitfields, blobs, etc), but also I don't like the idea of having a way to set arbitrary KMS props behind my back, which would be a very easy way to mess up the KMS state and cause conflicts when the compositor is upgraded.
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
Hi, Reviving this thread because I'm not sure what the outcome was. On Thu, Feb 29, 2024 at 11:52:12AM GMT, Daniel Vetter wrote: > > The only thing I'm saying is that this breaks the usual DRM requirements. > > If, as a maintainer, you're fine with breaking the rules and have a good > > motivation to do so, that's fine by me. Rules are meant to be broken from > > time to time depending on the situation. But please don't pretend that > > modetest/xrandr is valid user-space to pass the rules. > > I think it bends it pretty badly, because people running native Xorg are > slowly going away, and the modetest hack does not clear the bar for "is it > a joke/test/demo hack" for me. > > I think some weston (or whatever compositor you like) config file support > to set a bunch of "really only way to configure is by hand" output > properties would clear the bar here for me. Because that is a feature I > already mentioned that xrandr _does_ have, and which modetest hackery very > much does not. The expectation (and general usage) for that property was that it was set by the kernel command line and then was forgotten about. Old TVs require one mode and that's it, so it doesn't make much sense to change it while the system is live, you just want the default to work. So it's not really a matter of "the user-space code should be open" here, there's no user-space code, and there will likely never be given that it's mostly used to deal with decades-old systems at this point. Maxime signature.asc Description: PGP signature
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
Hi Simon, On Wed, Feb 28, 2024 at 04:22:56PM +, Simon Ser wrote: > On Wednesday, February 28th, 2024 at 17:14, Maxime Ripard > wrote: > > > > I don't know what the rules were 8 years ago, but the current uAPI rules > > > are what they are, and a new enum entry is new uAPI. > > > > TBF, and even if the wayland compositors support is missing, this > > property is perfectly usable as it is with upstream, open-source code, > > through either the command-line or X.org, and it's documented. > > > > So it's fine by me from a UAPI requirement side. > > That is not a valid way to pass the uAPI requirements IMHO. Yes, one > can program any KMS property via modetest or xrandr. Does that mean that > none of the new uAPI need a "real" implementation anymore? Does that mean > that the massive patch adding a color pipeline uAPI doesn't need > user-space anymore? I guess it's also a matter what the API is exactly? Like, xrandr or the kernel command line allows to use that particular API fully. Can you fully exert the color pipeline uAPI with xrandr? And at the time we submitted it, even with our best intent, we couldn't totally clear the userspace requirement because the PR would have been rejected because nobody wanted to deal with analog TV. And that's fair, any upstream project is free to do as it wants and analog TV is certainly not the state of the art anymore. But we had some variation of that property used in many drivers (i915, nouveau, vc4, sun4i and amlogic from the top of my head), all drivers-specific, and having that kind of support was also one of the blockers to move the few remaining fbdev drivers to KMS. It seems a bit unfair to put that requirement now that maybe some compositors could be interested. > The only thing I'm saying is that this breaks the usual DRM requirements. > If, as a maintainer, you're fine with breaking the rules and have a good > motivation to do so, that's fine by me. Rules are meant to be broken from > time to time depending on the situation. But please don't pretend that > modetest/xrandr is valid user-space to pass the rules. Ack. And indeed, modetest surely was a bad example. Maxime signature.asc Description: PGP signature
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Wed, Feb 28, 2024 at 04:22:56PM +, Simon Ser wrote: > On Wednesday, February 28th, 2024 at 17:14, Maxime Ripard > wrote: > > > > I don't know what the rules were 8 years ago, but the current uAPI rules > > > are what they are, and a new enum entry is new uAPI. > > > > TBF, and even if the wayland compositors support is missing, this > > property is perfectly usable as it is with upstream, open-source code, > > through either the command-line or X.org, and it's documented. > > > > So it's fine by me from a UAPI requirement side. > > That is not a valid way to pass the uAPI requirements IMHO. Yes, one > can program any KMS property via modetest or xrandr. Does that mean that > none of the new uAPI need a "real" implementation anymore? Does that mean > that the massive patch adding a color pipeline uAPI doesn't need > user-space anymore? xrandr only supports properties on the connector, so it's right out for the color pipeline. Also "we use xrandr for color properties" very much doesn't pass the bs filter of "is it a toy". My take would be that this escape hatch is also not valid for all connector property, stuff that is clearly meant to be configured automatically by the compositors cannot use the "we use xrandr" excuse, because users can't type fast enough and hit precisely enough to update a property in lockstep with the compositor's redraw loop :-) > The only thing I'm saying is that this breaks the usual DRM requirements. > If, as a maintainer, you're fine with breaking the rules and have a good > motivation to do so, that's fine by me. Rules are meant to be broken from > time to time depending on the situation. But please don't pretend that > modetest/xrandr is valid user-space to pass the rules. I think it bends it pretty badly, because people running native Xorg are slowly going away, and the modetest hack does not clear the bar for "is it a joke/test/demo hack" for me. I think some weston (or whatever compositor you like) config file support to set a bunch of "really only way to configure is by hand" output properties would clear the bar here for me. Because that is a feature I already mentioned that xrandr _does_ have, and which modetest hackery very much does not. -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Wednesday, February 28th, 2024 at 17:14, Maxime Ripard wrote: > > I don't know what the rules were 8 years ago, but the current uAPI rules > > are what they are, and a new enum entry is new uAPI. > > TBF, and even if the wayland compositors support is missing, this > property is perfectly usable as it is with upstream, open-source code, > through either the command-line or X.org, and it's documented. > > So it's fine by me from a UAPI requirement side. That is not a valid way to pass the uAPI requirements IMHO. Yes, one can program any KMS property via modetest or xrandr. Does that mean that none of the new uAPI need a "real" implementation anymore? Does that mean that the massive patch adding a color pipeline uAPI doesn't need user-space anymore? The only thing I'm saying is that this breaks the usual DRM requirements. If, as a maintainer, you're fine with breaking the rules and have a good motivation to do so, that's fine by me. Rules are meant to be broken from time to time depending on the situation. But please don't pretend that modetest/xrandr is valid user-space to pass the rules.
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
Hi, On Tue, Feb 27, 2024 at 09:51:06AM +, Simon Ser wrote: > On Monday, February 26th, 2024 at 18:23, Dave Stevenson > wrote: > > > and I've also completely missed any kernel command line > > > arguments manipulating KMS properties. > > > > "tv_mode" on the command line is handled in > > drm_mode_parse_cmdline_options() [3], as are rotate, reflect_x, > > reflect_y, margin_[left|right|top|bottom], and panel_orientation all > > to set the relevant KMS properties. > > > > Having "video=Composite-1:PAL,tv_mode=Mono" on the kernel command line > > will set up connector Composite-1 with the standard 720x576 50Hz > > interlaced timings, and DRM_MODE_TV_MODE_MONOCHROME selected on the > > tv_mode property. Swap in different tv_mode descriptions as required > > (eg PAL,tv_mode=SECAM), although some make little sense. > > > > That's the main route I'm looking at for configuring this property, > > for situations such as having a black and white TV connected. You > > don't get the opportunity to interrogate a composite display over what > > it supports, so it has to be configured manually somewhere in the > > system. If your monitor doesn't support the system default, then you > > can't see a GUI in order to change the option, and there is no > > guaranteed supported configuration so the command line is about the > > only option. > > > > The use cases for runtime switching of the "tv_mode" are exceedingly > > rare, so IMHO the property doesn't need exposing through the UAPI. > > However it was added to the UAPI about 8 years ago for vc4 and sunxi, > > and is also now used by other drivers, so can't be reverted. Does that > > mean it can now never be changed without jumping through the hoop of > > creating some userspace user? > > I don't know what the rules were 8 years ago, but the current uAPI rules > are what they are, and a new enum entry is new uAPI. TBF, and even if the wayland compositors support is missing, this property is perfectly usable as it is with upstream, open-source code, through either the command-line or X.org, and it's documented. So it's fine by me from a UAPI requirement side. Maxime signature.asc Description: PGP signature
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Mon, 26 Feb 2024 17:23:24 + Dave Stevenson wrote: > Hi Pekka > > Sorry for the slight delay in replying. > > On Mon, 26 Feb 2024 at 15:11, Pekka Paalanen > wrote: > > > > On Mon, 26 Feb 2024 15:10:45 +0100 > > Maxime Ripard wrote: > > > > > Hi Pekka, > > > > > > On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > > > > On Fri, 16 Feb 2024 18:48:55 + > > > > Dave Stevenson wrote: > > > > > > > > > From: Nick Hollinghurst > > > > > > > > > > Add this as a value for enum_drm_connector_tv_mode, represented > > > > > by the string "Mono", to generate video with no colour encoding > > > > > or bursts. Define it to have no pedestal (since only NTSC-M calls > > > > > for a pedestal). > > > > > > > > > > Change default mode creation to acommodate the new tv_mode value > > > > > which comprises both 525-line and 625-line formats. > > > > > > > > > > Signed-off-by: Nick Hollinghurst > > > > > Signed-off-by: Dave Stevenson > > > > > > > > since no-one else commented yet, I'd like to remind of the new UAPI > > > > requirements: > > > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > > > > > > > > AFAIU, adding a new value to an existing enum still counts as new UAPI. > > > > > > > > Maybe there is no need for the full treatment here, or maybe there is, > > > > I'm not sure. I think you should make some statement about how the new > > > > UAPI requirements have been addressed. ... > The use cases for runtime switching of the "tv_mode" are exceedingly > rare, so IMHO the property doesn't need exposing through the UAPI. > However it was added to the UAPI about 8 years ago for vc4 and sunxi, > and is also now used by other drivers, so can't be reverted. Does that > mean it can now never be changed without jumping through the hoop of > creating some userspace user? That is for the DRM maintainers to decide. For that, they must first notice that this is indeed new UAPI. History has shown that UAPI changes sometimes get through when they would have probably been rejected off-hand if a maintainer had a proper look. I saw an UAPI addition that was not in any way highlighted as such, with a topic that is probably uninteresting to most. The patch also did not discuss any of the details you now explained, which could serve as a justification. Naturally I screamed, hoping to attract maintainer attention. Thanks, pq > > Regards > Dave > > [3] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L2232 pgpmvGE0RBILi.pgp Description: OpenPGP digital signature
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Monday, February 26th, 2024 at 18:23, Dave Stevenson wrote: > You want the commit text for a patch adding a new enum to state that > the whole property isn't expected to be used through the UAPI? OK, I > can roll a v2 if that is your desire. By definition, anything new exposed by the kernel via KMS is new uAPI, even if it's not expected to be used by most compositors. > > You should expect that all KMS clients will work towards programming > > all exposed KMS properties into known values. That's the only way to > > achieve repeatable KMS behaviour, because there is no KMS reset ioctl. > > > > There are no two tiers of KMS properties AFAIK. You have to be the DRM > > master to change anything. So, people cannot force any settings from > > outside of a KMS client, they have to go through the KMS client, like > > xrandr goes through Xorg (and only Xorg). > > > > I do fully expect Weston to gain support for this property, if anyone > > cares of its value. That goes for all Wayland compositors. > > I don't know about Weston, but Wayfire / wlroots / sway have currently > chosen to ignore all interlaced display modes [1]. > [2] is the wlroots devs basically calling interlaced output a dead end. Note, part of this thread is missing on GitLab: https://github.com/swaywm/wlroots/issues/3038 Also note that none of that is set in stone if someone comes with a solid enough use-case. > That makes the debate for controlling the colour encoding on a > composite video rather redundant as they're almost always interlaced. > > [1] https://github.com/swaywm/sway/issues/3167 > [2] https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3038 > > > You're correct in that a KMS client would probably not know to control > > the value of this property automatically but it needs to come from > > configuration. That would be each KMS client's configuration. I don't > > understand how a script could achieve that. > > > > However, if you feel it is important to have KMS properties that > > display servers must not touch, then they should be documented as such. > > I do not know if that would actually lift the new-UAPI requirements, > > that is for the DRM maintainers to decide and document. Is there such a > > thing already? > > > > What are those "similar to xrandr" mechanisms? I don't think I've heard > > of any, > > Boot to the console and run > modetest -w :"tv_mode":"NTSC" > > There is no reset mechanism for all properties, so that setting > persists after modetest quits. That is a hack. This hack is no guarantee to continue to work in the future. KMS is not intended to be abused this way. There is a single component in charge of managing the KMS properties: the compositor. Any attempt to bypass it only works by chance, if it works at all and doesn't result in fireworks. > > and I've also completely missed any kernel command line > > arguments manipulating KMS properties. > > "tv_mode" on the command line is handled in > drm_mode_parse_cmdline_options() [3], as are rotate, reflect_x, > reflect_y, margin_[left|right|top|bottom], and panel_orientation all > to set the relevant KMS properties. > > Having "video=Composite-1:PAL,tv_mode=Mono" on the kernel command line > will set up connector Composite-1 with the standard 720x576 50Hz > interlaced timings, and DRM_MODE_TV_MODE_MONOCHROME selected on the > tv_mode property. Swap in different tv_mode descriptions as required > (eg PAL,tv_mode=SECAM), although some make little sense. > > That's the main route I'm looking at for configuring this property, > for situations such as having a black and white TV connected. You > don't get the opportunity to interrogate a composite display over what > it supports, so it has to be configured manually somewhere in the > system. If your monitor doesn't support the system default, then you > can't see a GUI in order to change the option, and there is no > guaranteed supported configuration so the command line is about the > only option. > > The use cases for runtime switching of the "tv_mode" are exceedingly > rare, so IMHO the property doesn't need exposing through the UAPI. > However it was added to the UAPI about 8 years ago for vc4 and sunxi, > and is also now used by other drivers, so can't be reverted. Does that > mean it can now never be changed without jumping through the hoop of > creating some userspace user? I don't know what the rules were 8 years ago, but the current uAPI rules are what they are, and a new enum entry is new uAPI.
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
Hi Pekka Sorry for the slight delay in replying. On Mon, 26 Feb 2024 at 15:11, Pekka Paalanen wrote: > > On Mon, 26 Feb 2024 15:10:45 +0100 > Maxime Ripard wrote: > > > Hi Pekka, > > > > On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > > > On Fri, 16 Feb 2024 18:48:55 + > > > Dave Stevenson wrote: > > > > > > > From: Nick Hollinghurst > > > > > > > > Add this as a value for enum_drm_connector_tv_mode, represented > > > > by the string "Mono", to generate video with no colour encoding > > > > or bursts. Define it to have no pedestal (since only NTSC-M calls > > > > for a pedestal). > > > > > > > > Change default mode creation to acommodate the new tv_mode value > > > > which comprises both 525-line and 625-line formats. > > > > > > > > Signed-off-by: Nick Hollinghurst > > > > Signed-off-by: Dave Stevenson > > > > > > since no-one else commented yet, I'd like to remind of the new UAPI > > > requirements: > > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > > > > > > AFAIU, adding a new value to an existing enum still counts as new UAPI. > > > > > > Maybe there is no need for the full treatment here, or maybe there is, > > > I'm not sure. I think you should make some statement about how the new > > > UAPI requirements have been addressed. > > > > That property was meant to provide legacy display handling, so I don't > > expect any reasonably recent codebase like Weston to suppport it, ever > > :) > > > > That being said, from the beginning, that property was meant to be > > handled as a "mode-setting" property, and thus handled by either the > > kernel command-line, xrandr, or any similar mechanism. > > > > I would expect that new enum variant to be handled under the same terms: > > it'll probably only show up in distro scripts or configuration files, > > and never in any actual code base. > > > > Is it what you were expecting, or did you mean something else? > > Maybe? Let's have it in the commit message and see if DRM maintainers > agree. You want the commit text for a patch adding a new enum to state that the whole property isn't expected to be used through the UAPI? OK, I can roll a v2 if that is your desire. > You should expect that all KMS clients will work towards programming > all exposed KMS properties into known values. That's the only way to > achieve repeatable KMS behaviour, because there is no KMS reset ioctl. > > There are no two tiers of KMS properties AFAIK. You have to be the DRM > master to change anything. So, people cannot force any settings from > outside of a KMS client, they have to go through the KMS client, like > xrandr goes through Xorg (and only Xorg). > > I do fully expect Weston to gain support for this property, if anyone > cares of its value. That goes for all Wayland compositors. I don't know about Weston, but Wayfire / wlroots / sway have currently chosen to ignore all interlaced display modes [1]. [2] is the wlroots devs basically calling interlaced output a dead end. That makes the debate for controlling the colour encoding on a composite video rather redundant as they're almost always interlaced. [1] https://github.com/swaywm/sway/issues/3167 [2] https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3038 > You're correct in that a KMS client would probably not know to control > the value of this property automatically but it needs to come from > configuration. That would be each KMS client's configuration. I don't > understand how a script could achieve that. > > However, if you feel it is important to have KMS properties that > display servers must not touch, then they should be documented as such. > I do not know if that would actually lift the new-UAPI requirements, > that is for the DRM maintainers to decide and document. Is there such a > thing already? > > What are those "similar to xrandr" mechanisms? I don't think I've heard > of any, Boot to the console and run modetest -w :"tv_mode":"NTSC" There is no reset mechanism for all properties, so that setting persists after modetest quits. > and I've also completely missed any kernel command line > arguments manipulating KMS properties. "tv_mode" on the command line is handled in drm_mode_parse_cmdline_options() [3], as are rotate, reflect_x, reflect_y, margin_[left|right|top|bottom], and panel_orientation all to set the relevant KMS properties. Having "video=Composite-1:PAL,tv_mode=Mono" on the kernel command line will set up connector Composite-1 with the standard 720x576 50Hz interlaced timings, and DRM_MODE_TV_MODE_MONOCHROME selected on the tv_mode property. Swap in different tv_mode descriptions as required (eg PAL,tv_mode=SECAM), although some make little sense. That's the main route I'm looking at for configuring this property, for situations such as having a black and white TV connected. You don't get the opportunity to interrogate a composite display over what it supports, so it has to be configured manually somewhere in
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Mon, 26 Feb 2024 15:10:45 +0100 Maxime Ripard wrote: > Hi Pekka, > > On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > > On Fri, 16 Feb 2024 18:48:55 + > > Dave Stevenson wrote: > > > > > From: Nick Hollinghurst > > > > > > Add this as a value for enum_drm_connector_tv_mode, represented > > > by the string "Mono", to generate video with no colour encoding > > > or bursts. Define it to have no pedestal (since only NTSC-M calls > > > for a pedestal). > > > > > > Change default mode creation to acommodate the new tv_mode value > > > which comprises both 525-line and 625-line formats. > > > > > > Signed-off-by: Nick Hollinghurst > > > Signed-off-by: Dave Stevenson > > > > since no-one else commented yet, I'd like to remind of the new UAPI > > requirements: > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > > > > AFAIU, adding a new value to an existing enum still counts as new UAPI. > > > > Maybe there is no need for the full treatment here, or maybe there is, > > I'm not sure. I think you should make some statement about how the new > > UAPI requirements have been addressed. > > That property was meant to provide legacy display handling, so I don't > expect any reasonably recent codebase like Weston to suppport it, ever > :) > > That being said, from the beginning, that property was meant to be > handled as a "mode-setting" property, and thus handled by either the > kernel command-line, xrandr, or any similar mechanism. > > I would expect that new enum variant to be handled under the same terms: > it'll probably only show up in distro scripts or configuration files, > and never in any actual code base. > > Is it what you were expecting, or did you mean something else? Maybe? Let's have it in the commit message and see if DRM maintainers agree. You should expect that all KMS clients will work towards programming all exposed KMS properties into known values. That's the only way to achieve repeatable KMS behaviour, because there is no KMS reset ioctl. There are no two tiers of KMS properties AFAIK. You have to be the DRM master to change anything. So, people cannot force any settings from outside of a KMS client, they have to go through the KMS client, like xrandr goes through Xorg (and only Xorg). I do fully expect Weston to gain support for this property, if anyone cares of its value. That goes for all Wayland compositors. You're correct in that a KMS client would probably not know to control the value of this property automatically but it needs to come from configuration. That would be each KMS client's configuration. I don't understand how a script could achieve that. However, if you feel it is important to have KMS properties that display servers must not touch, then they should be documented as such. I do not know if that would actually lift the new-UAPI requirements, that is for the DRM maintainers to decide and document. Is there such a thing already? What are those "similar to xrandr" mechanisms? I don't think I've heard of any, and I've also completely missed any kernel command line arguments manipulating KMS properties. Thanks, pq pgpfKLBOKnvHP.pgp Description: OpenPGP digital signature
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
Hi Pekka, On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > On Fri, 16 Feb 2024 18:48:55 + > Dave Stevenson wrote: > > > From: Nick Hollinghurst > > > > Add this as a value for enum_drm_connector_tv_mode, represented > > by the string "Mono", to generate video with no colour encoding > > or bursts. Define it to have no pedestal (since only NTSC-M calls > > for a pedestal). > > > > Change default mode creation to acommodate the new tv_mode value > > which comprises both 525-line and 625-line formats. > > > > Signed-off-by: Nick Hollinghurst > > Signed-off-by: Dave Stevenson > > since no-one else commented yet, I'd like to remind of the new UAPI > requirements: > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > > AFAIU, adding a new value to an existing enum still counts as new UAPI. > > Maybe there is no need for the full treatment here, or maybe there is, > I'm not sure. I think you should make some statement about how the new > UAPI requirements have been addressed. That property was meant to provide legacy display handling, so I don't expect any reasonably recent codebase like Weston to suppport it, ever :) That being said, from the beginning, that property was meant to be handled as a "mode-setting" property, and thus handled by either the kernel command-line, xrandr, or any similar mechanism. I would expect that new enum variant to be handled under the same terms: it'll probably only show up in distro scripts or configuration files, and never in any actual code base. Is it what you were expecting, or did you mean something else? Maxime signature.asc Description: PGP signature
Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On 2024-02-21 04:07, Pekka Paalanen wrote:
> On Fri, 16 Feb 2024 18:48:55 +
> Dave Stevenson wrote:
>
>> From: Nick Hollinghurst
>>
>> Add this as a value for enum_drm_connector_tv_mode, represented
>> by the string "Mono", to generate video with no colour encoding
>> or bursts. Define it to have no pedestal (since only NTSC-M calls
>> for a pedestal).
>>
>> Change default mode creation to acommodate the new tv_mode value
>> which comprises both 525-line and 625-line formats.
>>
>> Signed-off-by: Nick Hollinghurst
>> Signed-off-by: Dave Stevenson
>
> Hi Dave and Nick,
>
> since no-one else commented yet, I'd like to remind of the new UAPI
> requirements:
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>
> AFAIU, adding a new value to an existing enum still counts as new UAPI.
>
I tend to agree with Pekka. I'm getting tired of seeing new DRM properties
without knowing which canonical upstream user-space project uses it.
Can someone describe this for the "TV mode" property?
Harry
> Maybe there is no need for the full treatment here, or maybe there is,
> I'm not sure. I think you should make some statement about how the new
> UAPI requirements have been addressed.
>
> Btw. no-one has submitted a record with "TV mode" to
> https://drmdb.emersion.fr/
> It only lists the radeon-specific "tv standard" property. I first
> thought you had mistaken the property name in the cover letter.
>
>
> Thanks,
> pq
>
>> ---
>> drivers/gpu/drm/drm_connector.c| 7 +++
>> drivers/gpu/drm/drm_modes.c| 5 -
>> drivers/gpu/drm/drm_probe_helper.c | 5 +++--
>> include/drm/drm_connector.h| 7 +++
>> 4 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c
>> index b0516505f7ae..fe05d27f3404 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1005,6 +1005,7 @@ static const struct drm_prop_enum_list
>> drm_tv_mode_enum_list[] = {
>> { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
>> { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
>> { DRM_MODE_TV_MODE_SECAM, "SECAM" },
>> +{ DRM_MODE_TV_MODE_MONOCHROME, "Mono" },
>> };
>> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>>
>> @@ -1697,6 +1698,12 @@
>> EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>> * TV Mode is CCIR System B (aka 625-lines) together with
>> * the SECAM Color Encoding.
>> *
>> + * Mono:
>> + *
>> + * Use timings appropriate to the DRM mode, including
>> + * equalizing pulses for a 525-line or 625-line mode,
>> + * with no pedestal or color encoding.
>> + *
>> * Drivers can set up this property by calling
>> * drm_mode_create_tv_properties().
>> */
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index c4f88c3a93b7..d274e7b00b79 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -530,7 +530,8 @@ static int fill_analog_mode(struct drm_device *dev,
>> * @interlace: whether to compute an interlaced mode
>> *
>> * This function creates a struct drm_display_mode instance suited for
>> - * an analog TV output, for one of the usual analog TV mode.
>> + * an analog TV output, for one of the usual analog TV modes. Where
>> + * this is DRM_MODE_TV_MODE_MONOCHROME, a 625-line mode will be created.
>> *
>> * Note that @hdisplay is larger than the usual constraints for the PAL
>> * and NTSC timings, and we'll choose to ignore most timings constraints
>> @@ -568,6 +569,8 @@ struct drm_display_mode *drm_analog_tv_mode(struct
>> drm_device *dev,
>> case DRM_MODE_TV_MODE_PAL_N:
>> fallthrough;
>> case DRM_MODE_TV_MODE_SECAM:
>> +fallthrough;
>> +case DRM_MODE_TV_MODE_MONOCHROME:
>> analog = DRM_MODE_ANALOG_PAL;
>> break;
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>> b/drivers/gpu/drm/drm_probe_helper.c
>> index d1e1ade66f81..9254dc2af873 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -1211,8 +1211,9 @@ int drm_connector_helper_tv_get_modes(struct
>> drm_connector *connector)
>> for (i = 0; i < tv_mode_property->num_values; i++)
>> supported_tv_modes |= BIT(tv_mode_property->values[i]);
>>
>> -if ((supported_tv_modes & ntsc_modes) &&
>> -(supported_tv_modes & pal_modes)) {
>> +if (((supported_tv_modes & ntsc_modes) &&
>> + (supported_tv_modes & pal_modes)) ||
>> +(supported_tv_modes & BIT(DRM_MODE_TV_MODE_MONOCHROME))) {
>> uint64_t default_mode;
>>
>> if (drm_object_property_get_default_value(>base,
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index fe88d7fc6b8f..90fd0ea0ca09 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
On Fri, 16 Feb 2024 18:48:55 +
Dave Stevenson wrote:
> From: Nick Hollinghurst
>
> Add this as a value for enum_drm_connector_tv_mode, represented
> by the string "Mono", to generate video with no colour encoding
> or bursts. Define it to have no pedestal (since only NTSC-M calls
> for a pedestal).
>
> Change default mode creation to acommodate the new tv_mode value
> which comprises both 525-line and 625-line formats.
>
> Signed-off-by: Nick Hollinghurst
> Signed-off-by: Dave Stevenson
Hi Dave and Nick,
since no-one else commented yet, I'd like to remind of the new UAPI
requirements:
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
AFAIU, adding a new value to an existing enum still counts as new UAPI.
Maybe there is no need for the full treatment here, or maybe there is,
I'm not sure. I think you should make some statement about how the new
UAPI requirements have been addressed.
Btw. no-one has submitted a record with "TV mode" to
https://drmdb.emersion.fr/
It only lists the radeon-specific "tv standard" property. I first
thought you had mistaken the property name in the cover letter.
Thanks,
pq
> ---
> drivers/gpu/drm/drm_connector.c| 7 +++
> drivers/gpu/drm/drm_modes.c| 5 -
> drivers/gpu/drm/drm_probe_helper.c | 5 +++--
> include/drm/drm_connector.h| 7 +++
> 4 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae..fe05d27f3404 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1005,6 +1005,7 @@ static const struct drm_prop_enum_list
> drm_tv_mode_enum_list[] = {
> { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> { DRM_MODE_TV_MODE_SECAM, "SECAM" },
> + { DRM_MODE_TV_MODE_MONOCHROME, "Mono" },
> };
> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>
> @@ -1697,6 +1698,12 @@
> EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
> * TV Mode is CCIR System B (aka 625-lines) together with
> * the SECAM Color Encoding.
> *
> + * Mono:
> + *
> + * Use timings appropriate to the DRM mode, including
> + * equalizing pulses for a 525-line or 625-line mode,
> + * with no pedestal or color encoding.
> + *
> * Drivers can set up this property by calling
> * drm_mode_create_tv_properties().
> */
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c4f88c3a93b7..d274e7b00b79 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -530,7 +530,8 @@ static int fill_analog_mode(struct drm_device *dev,
> * @interlace: whether to compute an interlaced mode
> *
> * This function creates a struct drm_display_mode instance suited for
> - * an analog TV output, for one of the usual analog TV mode.
> + * an analog TV output, for one of the usual analog TV modes. Where
> + * this is DRM_MODE_TV_MODE_MONOCHROME, a 625-line mode will be created.
> *
> * Note that @hdisplay is larger than the usual constraints for the PAL
> * and NTSC timings, and we'll choose to ignore most timings constraints
> @@ -568,6 +569,8 @@ struct drm_display_mode *drm_analog_tv_mode(struct
> drm_device *dev,
> case DRM_MODE_TV_MODE_PAL_N:
> fallthrough;
> case DRM_MODE_TV_MODE_SECAM:
> + fallthrough;
> + case DRM_MODE_TV_MODE_MONOCHROME:
> analog = DRM_MODE_ANALOG_PAL;
> break;
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index d1e1ade66f81..9254dc2af873 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1211,8 +1211,9 @@ int drm_connector_helper_tv_get_modes(struct
> drm_connector *connector)
> for (i = 0; i < tv_mode_property->num_values; i++)
> supported_tv_modes |= BIT(tv_mode_property->values[i]);
>
> - if ((supported_tv_modes & ntsc_modes) &&
> - (supported_tv_modes & pal_modes)) {
> + if (((supported_tv_modes & ntsc_modes) &&
> + (supported_tv_modes & pal_modes)) ||
> + (supported_tv_modes & BIT(DRM_MODE_TV_MODE_MONOCHROME))) {
> uint64_t default_mode;
>
> if (drm_object_property_get_default_value(>base,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f..90fd0ea0ca09 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -200,6 +200,13 @@ enum drm_connector_tv_mode {
>*/
> DRM_MODE_TV_MODE_SECAM,
>
> + /**
> + * @DRM_MODE_TV_MODE_MONOCHROME: Use timings appropriate to
> + * the DRM mode, including equalizing pulses for a 525-line
> + * or 625-line mode, with no pedestal or color encoding.
> + */
> + DRM_MODE_TV_MODE_MONOCHROME,
> +
>
[PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME
From: Nick Hollinghurst
Add this as a value for enum_drm_connector_tv_mode, represented
by the string "Mono", to generate video with no colour encoding
or bursts. Define it to have no pedestal (since only NTSC-M calls
for a pedestal).
Change default mode creation to acommodate the new tv_mode value
which comprises both 525-line and 625-line formats.
Signed-off-by: Nick Hollinghurst
Signed-off-by: Dave Stevenson
---
drivers/gpu/drm/drm_connector.c| 7 +++
drivers/gpu/drm/drm_modes.c| 5 -
drivers/gpu/drm/drm_probe_helper.c | 5 +++--
include/drm/drm_connector.h| 7 +++
4 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b0516505f7ae..fe05d27f3404 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1005,6 +1005,7 @@ static const struct drm_prop_enum_list
drm_tv_mode_enum_list[] = {
{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
{ DRM_MODE_TV_MODE_SECAM, "SECAM" },
+ { DRM_MODE_TV_MODE_MONOCHROME, "Mono" },
};
DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
@@ -1697,6 +1698,12 @@
EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
* TV Mode is CCIR System B (aka 625-lines) together with
* the SECAM Color Encoding.
*
+ * Mono:
+ *
+ * Use timings appropriate to the DRM mode, including
+ * equalizing pulses for a 525-line or 625-line mode,
+ * with no pedestal or color encoding.
+ *
* Drivers can set up this property by calling
* drm_mode_create_tv_properties().
*/
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index c4f88c3a93b7..d274e7b00b79 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -530,7 +530,8 @@ static int fill_analog_mode(struct drm_device *dev,
* @interlace: whether to compute an interlaced mode
*
* This function creates a struct drm_display_mode instance suited for
- * an analog TV output, for one of the usual analog TV mode.
+ * an analog TV output, for one of the usual analog TV modes. Where
+ * this is DRM_MODE_TV_MODE_MONOCHROME, a 625-line mode will be created.
*
* Note that @hdisplay is larger than the usual constraints for the PAL
* and NTSC timings, and we'll choose to ignore most timings constraints
@@ -568,6 +569,8 @@ struct drm_display_mode *drm_analog_tv_mode(struct
drm_device *dev,
case DRM_MODE_TV_MODE_PAL_N:
fallthrough;
case DRM_MODE_TV_MODE_SECAM:
+ fallthrough;
+ case DRM_MODE_TV_MODE_MONOCHROME:
analog = DRM_MODE_ANALOG_PAL;
break;
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index d1e1ade66f81..9254dc2af873 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1211,8 +1211,9 @@ int drm_connector_helper_tv_get_modes(struct
drm_connector *connector)
for (i = 0; i < tv_mode_property->num_values; i++)
supported_tv_modes |= BIT(tv_mode_property->values[i]);
- if ((supported_tv_modes & ntsc_modes) &&
- (supported_tv_modes & pal_modes)) {
+ if (((supported_tv_modes & ntsc_modes) &&
+(supported_tv_modes & pal_modes)) ||
+ (supported_tv_modes & BIT(DRM_MODE_TV_MODE_MONOCHROME))) {
uint64_t default_mode;
if (drm_object_property_get_default_value(>base,
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index fe88d7fc6b8f..90fd0ea0ca09 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -200,6 +200,13 @@ enum drm_connector_tv_mode {
*/
DRM_MODE_TV_MODE_SECAM,
+ /**
+* @DRM_MODE_TV_MODE_MONOCHROME: Use timings appropriate to
+* the DRM mode, including equalizing pulses for a 525-line
+* or 625-line mode, with no pedestal or color encoding.
+*/
+ DRM_MODE_TV_MODE_MONOCHROME,
+
/**
* @DRM_MODE_TV_MODE_MAX: Number of analog TV output modes.
*
--
2.25.1
