| Message ID | 20240216184857.245372-2-dave.stevenson@raspberrypi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vc4 VEC (analogue video) updates - margins and monochrome | expand |
On Fri, 16 Feb 2024 18:48:55 +0000 Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > 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 <nick.hollinghurst@raspberrypi.com> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> 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(&connector->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. > *
On 2024-02-21 04:07, Pekka Paalanen wrote: > On Fri, 16 Feb 2024 18:48:55 +0000 > Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > >> From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> >> >> 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 <nick.hollinghurst@raspberrypi.com> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > 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(&connector->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. >> * >
Hi Pekka, On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > On Fri, 16 Feb 2024 18:48:55 +0000 > Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > > From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > 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 <nick.hollinghurst@raspberrypi.com> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > 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
On Mon, 26 Feb 2024 15:10:45 +0100 Maxime Ripard <mripard@kernel.org> wrote: > Hi Pekka, > > On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > > On Fri, 16 Feb 2024 18:48:55 +0000 > > Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > > > > From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > > > 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 <nick.hollinghurst@raspberrypi.com> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > 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
Hi Pekka Sorry for the slight delay in replying. On Mon, 26 Feb 2024 at 15:11, Pekka Paalanen <pekka.paalanen@haloniitty.fi> wrote: > > On Mon, 26 Feb 2024 15:10:45 +0100 > Maxime Ripard <mripard@kernel.org> wrote: > > > Hi Pekka, > > > > On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > > > On Fri, 16 Feb 2024 18:48:55 +0000 > > > Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > > > > > > From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > > > > > 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 <nick.hollinghurst@raspberrypi.com> > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > 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 <connector id>:"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 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? Regards Dave [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L2232
On Monday, February 26th, 2024 at 18:23, Dave Stevenson <dave.stevenson@raspberrypi.com> 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 <connector id>:"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.
On Mon, 26 Feb 2024 17:23:24 +0000 Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > Hi Pekka > > Sorry for the slight delay in replying. > > On Mon, 26 Feb 2024 at 15:11, Pekka Paalanen > <pekka.paalanen@haloniitty.fi> wrote: > > > > On Mon, 26 Feb 2024 15:10:45 +0100 > > Maxime Ripard <mripard@kernel.org> wrote: > > > > > Hi Pekka, > > > > > > On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > > > > On Fri, 16 Feb 2024 18:48:55 +0000 > > > > Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > > > > > > > > From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > > > > > > > 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 <nick.hollinghurst@raspberrypi.com> > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > > > 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
Hi, On Mon, Feb 26, 2024 at 05:11:43PM +0200, Pekka Paalanen wrote: > On Mon, 26 Feb 2024 15:10:45 +0100 > Maxime Ripard <mripard@kernel.org> wrote: > > > Hi Pekka, > > > > On Wed, Feb 21, 2024 at 11:07:51AM +0200, Pekka Paalanen wrote: > > > On Fri, 16 Feb 2024 18:48:55 +0000 > > > Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > > > > > > From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > > > > > 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 <nick.hollinghurst@raspberrypi.com> > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > 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. Ack. > 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). Sure. My point wasn't that we should magically get an exemption ... > I do fully expect Weston to gain support for this property, if anyone > cares of its value. That goes for all Wayland compositors. ... but rather that, back when we discussed it, this definitely isn't what the general feeling was. And that's fine, I'm glad it's something that Weston would consider. > 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. I meant a script calling xrandr at boot for example. > 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? It's not what I meant really. Again, back when we were discussing merging that property no KMS client we could discuss with really wanted to have anything to do with analog. So it's not that we were expecting to have a property that wasn't meant to be modified by a compositor, but rather that none wanted to. I fully expect that property to work and be something that compositors can modify if they want to. Maxime
On Wed, Feb 28, 2024 at 04:29:33PM +0100, Maxime Ripard wrote: > On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote: > > Add select_output_bus_format to CRTC atomic helpers callbacks. This > > callback Will allow CRTC to participate in media bus format negotiation > > over connected DRM bridge chain and impose CRTC-specific restrictions. > > A good example is CRTC implemented as FPGA soft IP. This kind of CRTC will > > most certainly support a single output media bus format, as supporting > > multiple runtime options consumes extra FPGA resources. A variety of > > options for FPGA are usually achieved by synthesizing IP with different > > parameters. > > > > Incorporate select_output_bus_format callback into the format negotiation > > stage to fix the input bus format of the first DRM bridge in the chain. > > > > Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@amd.com> > > --- > > drivers/gpu/drm/drm_bridge.c | 19 +++++++++++++++++-- > > include/drm/drm_modeset_helper_vtables.h | 31 +++++++++++++++++++++++++++++++ > > 2 files changed, 48 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index 521a71c61b16..453ae3d174b4 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -32,6 +32,7 @@ > > #include <drm/drm_edid.h> > > #include <drm/drm_encoder.h> > > #include <drm/drm_file.h> > > +#include <drm/drm_modeset_helper_vtables.h> > > #include <drm/drm_of.h> > > #include <drm/drm_print.h> > > > > @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, > > unsigned int i, num_in_bus_fmts = 0; > > struct drm_bridge_state *cur_state; > > struct drm_bridge *prev_bridge; > > - u32 *in_bus_fmts; > > + struct drm_crtc *crtc = crtc_state->crtc; > > + u32 *in_bus_fmts, in_fmt; > > int ret; > > > > prev_bridge = drm_bridge_get_prev_bridge(cur_bridge); > > @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, > > return -ENOMEM; > > > > if (first_bridge == cur_bridge) { > > - cur_state->input_bus_cfg.format = in_bus_fmts[0]; > > + in_fmt = in_bus_fmts[0]; > > + if (crtc->helper_private && > > + crtc->helper_private->select_output_bus_format) { > > + in_fmt = crtc->helper_private->select_output_bus_format( > > + crtc, > > + crtc->state, > > + in_bus_fmts, > > + num_in_bus_fmts); > > + if (!in_fmt) { > > + kfree(in_bus_fmts); > > + return -ENOTSUPP; > > + } > > + } > > + cur_state->input_bus_cfg.format = in_fmt; > > I don't think we should start poking at the CRTC internals, but we > should rather provide a helper here. It would probably look cleaner, yes. > > cur_state->output_bus_cfg.format = out_bus_fmt; > > kfree(in_bus_fmts); > > return 0; > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > > index 881b03e4dc28..7c21ae1fe3ad 100644 > > --- a/include/drm/drm_modeset_helper_vtables.h > > +++ b/include/drm/drm_modeset_helper_vtables.h > > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs { > > bool in_vblank_irq, int *vpos, int *hpos, > > ktime_t *stime, ktime_t *etime, > > const struct drm_display_mode *mode); > > + > > + /** > > + * @select_output_bus_format > > + * > > + * Called by the first connected DRM bridge to negotiate input media > > + * bus format. CRTC is expected to pick preferable media formats from > > + * the list supported by the DRM bridge chain. > > There's nothing restricting it to bridges here. Please rephrase this to > remove the bridge mention. The user is typically going to be the > encoder, and bridges are just an automagic implementation of an encoder. > > And generally speaking, I'd really like to have an implementation > available before merging this. There's a downstream implementation in the Xilinx kernel, which would indeed be nice to upstream. This shouldn't block patches 1/4 to 3/4 though, those can be merged once review comments are taken into account.
Hi, On Tue, Feb 27, 2024 at 09:51:06AM +0000, Simon Ser wrote: > On Monday, February 26th, 2024 at 18:23, Dave Stevenson <dave.stevenson@raspberrypi.com> 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
On Wednesday, February 28th, 2024 at 17:14, Maxime Ripard <mripard@kernel.org> 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.
On Wed, Feb 28, 2024 at 04:22:56PM +0000, Simon Ser wrote: > On Wednesday, February 28th, 2024 at 17:14, Maxime Ripard <mripard@kernel.org> 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 <Enter> 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
Hi Simon, On Wed, Feb 28, 2024 at 04:22:56PM +0000, Simon Ser wrote: > On Wednesday, February 28th, 2024 at 17:14, Maxime Ripard <mripard@kernel.org> 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
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
On Thursday, February 29th, 2024 at 11:52, Daniel Vetter <daniel@ffwll.ch> 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.
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
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
On Fri, 16 Feb 2024 18:48:55 +0000, 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
Hi, On Fri, Feb 16, 2024 at 06:48:55PM GMT, Dave Stevenson wrote: > From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > 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 <nick.hollinghurst@raspberrypi.com> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > 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
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(&connector->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. *