| Message ID | 20220728-rpi-analog-tv-properties-v5-20-d841cc64fe4b@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
Hi Maxime, > static int vc4_vec_connector_get_modes(struct drm_connector *connector) > { > - struct drm_connector_state *state = connector->state; > struct drm_display_mode *mode; > > - mode = drm_mode_duplicate(connector->dev, > - vc4_vec_tv_modes[state->tv.legacy_mode].mode); > + mode = drm_mode_analog_ntsc_480i(connector->dev); > if (!mode) { > DRM_ERROR("Failed to create a new display mode\n"); > return -ENOMEM; > } > > + mode->type |= DRM_MODE_TYPE_PREFERRED; > drm_mode_probed_add(connector, mode); > > - return 1; > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) { > + DRM_ERROR("Failed to create a new display mode\n"); > + return -ENOMEM; > + } > + > + drm_mode_probed_add(connector, mode); > + > + return 2; > +} Referencing those previous discussions: - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/ - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/ Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg (at least on current Raspberry Pi OS) to display garbage when video=Composite1:PAL is specified on the command line, so I'm afraid this won't do. As I see it, there are three viable solutions for this issue: a) Somehow query the video= command line option from this function, and set DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction provided by global DRM code, but should work fine. b) Modify drm_helper_probe_add_cmdline_mode() so that it sets DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems pretty robust, but affects the entire DRM subsystem, which may break userspace in different ways. - Maybe this could be mitigated by adding some additional conditions, e.g. setting the PREFERRED flag only if no modes are already flagged as such and/or only if the cmdline mode is a named one (~= analog TV mode) c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF flag. Either way, hardcoding 480i as PREFERRED does not seem right. Note: this also applies to the sun4i version (patch 22/22). > @@ -366,13 +472,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder, > struct drm_connector *connector = &vec->connector; > struct drm_connector_state *conn_state = > drm_atomic_get_new_connector_state(state, connector); > - const struct vc4_vec_tv_mode *tv_mode = > - &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > + const struct vc4_vec_tv_mode *tv_mode; > int idx, ret; > > if (!drm_dev_enter(drm, &idx)) > return; > > + tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode); > + if (!tv_mode) > + goto err_dev_exit; > + > ret = pm_runtime_get_sync(&vec->pdev->dev); > if (ret < 0) { > DRM_ERROR("Failed to retain power domain: %d\n", ret); If this (!tv_mode) condition is somehow triggered, the power management goes somewhat crazy. vc4_vec_encoder_enable() cannot return an error, so when vc4_vec_encoder_disable() is eventually called after a failed enable, it hangs in pm_runtime_put() for quite a bit. At least I think that's what's happening. Anyway, to solve this, I'd propose this thing below: Best regards, Mateusz Kwiatkowski
Hi Maxime, Urgh. I cannot send e-mails apparently today, as I removed the second half of the previous message. Here goes: > @@ -454,13 +563,6 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > struct drm_connector_state *conn_state) > { > const struct drm_display_mode *mode = &crtc_state->adjusted_mode; You could add here something like: + const struct vc4_vec_tv_mode *tv_mode = + vc4_vec_tv_mode_lookup(conn_state->tv.mode); + + if (!tv_mode) + return -EINVAL; This should explicitly make it impossible to enter the equivalent condition in vc4_vec_encoder_enable() that causes the problem mentioned in the previous e-mail. This is probably basically impossible already, but I triggered that when testing a follow-up change I'd like to post shortly. > - const struct vc4_vec_tv_mode *vec_mode; > - > - vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > - > - if (conn_state->crtc && > - !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > - return -EINVAL; If you're removing the reference mode, then I think you should at least add checks that the crtc_clock is set to 13.5 MHz (it's otherwise ignored) and that crtc_htotal is either 858 or 864 (using a switch over reference_mode->htotal as I proposed in my comment to patch 19/22 would double as such check), as all other values causes VEC to output garbage. Best regards, Mateusz Kwiatkowski
Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski: > Hi Maxime, > >> static int vc4_vec_connector_get_modes(struct drm_connector *connector) >> { >> - struct drm_connector_state *state = connector->state; >> struct drm_display_mode *mode; >> >> - mode = drm_mode_duplicate(connector->dev, >> - vc4_vec_tv_modes[state->tv.legacy_mode].mode); >> + mode = drm_mode_analog_ntsc_480i(connector->dev); >> if (!mode) { >> DRM_ERROR("Failed to create a new display mode\n"); >> return -ENOMEM; >> } >> >> + mode->type |= DRM_MODE_TYPE_PREFERRED; >> drm_mode_probed_add(connector, mode); >> >> - return 1; >> + mode = drm_mode_analog_pal_576i(connector->dev); >> + if (!mode) { >> + DRM_ERROR("Failed to create a new display mode\n"); >> + return -ENOMEM; >> + } >> + >> + drm_mode_probed_add(connector, mode); >> + >> + return 2; >> +} > > Referencing those previous discussions: > - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/ > - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/ > > Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg > (at least on current Raspberry Pi OS) to display garbage when > video=Composite1:PAL is specified on the command line, so I'm afraid this won't > do. > > As I see it, there are three viable solutions for this issue: > > a) Somehow query the video= command line option from this function, and set > DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction > provided by global DRM code, but should work fine. > > b) Modify drm_helper_probe_add_cmdline_mode() so that it sets > DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems > pretty robust, but affects the entire DRM subsystem, which may break > userspace in different ways. > > - Maybe this could be mitigated by adding some additional conditions, e.g. > setting the PREFERRED flag only if no modes are already flagged as such > and/or only if the cmdline mode is a named one (~= analog TV mode) > > c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF > flag. > > Either way, hardcoding 480i as PREFERRED does not seem right. > My solution for this is to look at tv.mode to know which mode to mark as preferred. Maxime didn't like this since it changes things behind userspace's back. I don't see how that can cause any problems for userspace. If userspace uses atomic and sets tv_mode, it has to know which mode to use before hand, so it doesn't look at the preferreded flag. If it uses legacy and sets tv_mode, it can end up with a stale preferred flag, but no worse than not having the flag or that ntsc is always preferred. If it doesn't change tv_mode, there's no problem, the preferred flag doesn't change. Noralf.
Den 13.10.2022 15.19, skrev Maxime Ripard: > Now that the core can deal fine with analog TV modes, let's convert the vc4 > VEC driver to leverage those new features. > > We've added some backward compatibility to support the old TV mode property > and translate it into the new TV norm property. We're also making use of > the new analog TV atomic_check helper to make sure we trigger a modeset > whenever the TV mode is updated. > > Acked-by: Noralf Trønnes <noralf@tronnes.org> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > @@ -276,19 +292,96 @@ static void vc4_vec_connector_reset(struct drm_connector *connector) > > static int vc4_vec_connector_get_modes(struct drm_connector *connector) > { > - struct drm_connector_state *state = connector->state; > struct drm_display_mode *mode; > > - mode = drm_mode_duplicate(connector->dev, > - vc4_vec_tv_modes[state->tv.legacy_mode].mode); > + mode = drm_mode_analog_ntsc_480i(connector->dev); > if (!mode) { > DRM_ERROR("Failed to create a new display mode\n"); > return -ENOMEM; > } > > + mode->type |= DRM_MODE_TYPE_PREFERRED; > drm_mode_probed_add(connector, mode); > > - return 1; > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) { > + DRM_ERROR("Failed to create a new display mode\n"); > + return -ENOMEM; I just remembered that you can't return an error from .get_modes, it should only return the number of modes added. From doc: * RETURNS: * * The number of modes added by calling drm_mode_probed_add(). See also the use of count in drm_helper_probe_single_connector_modes(). Patch 14 and 22 has the same issue. Noralf. > + } > + > + drm_mode_probed_add(connector, mode); > + > + return 2; > +} > + > +static int > +vc4_vec_connector_set_property(struct drm_connector *connector, > + struct drm_connector_state *state, > + struct drm_property *property, > + uint64_t val) > +{ > + struct vc4_vec *vec = connector_to_vc4_vec(connector); > + > + if (property != vec->legacy_tv_mode_property) > + return -EINVAL; > + > + switch (val) { > + case VC4_VEC_TV_MODE_NTSC: > + state->tv.mode = DRM_MODE_TV_MODE_NTSC; > + break; > + > + case VC4_VEC_TV_MODE_NTSC_J: > + state->tv.mode = DRM_MODE_TV_MODE_NTSC_J; > + break; > + > + case VC4_VEC_TV_MODE_PAL: > + state->tv.mode = DRM_MODE_TV_MODE_PAL; > + break; > + > + case VC4_VEC_TV_MODE_PAL_M: > + state->tv.mode = DRM_MODE_TV_MODE_PAL_M; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int > +vc4_vec_connector_get_property(struct drm_connector *connector, > + const struct drm_connector_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + struct vc4_vec *vec = connector_to_vc4_vec(connector); > + > + if (property != vec->legacy_tv_mode_property) > + return -EINVAL; > + > + switch (state->tv.mode) { > + case DRM_MODE_TV_MODE_NTSC: > + *val = VC4_VEC_TV_MODE_NTSC; > + break; > + > + case DRM_MODE_TV_MODE_NTSC_J: > + *val = VC4_VEC_TV_MODE_NTSC_J; > + break; > + > + case DRM_MODE_TV_MODE_PAL: > + *val = VC4_VEC_TV_MODE_PAL; > + break; > + > + case DRM_MODE_TV_MODE_PAL_M: > + *val = VC4_VEC_TV_MODE_PAL_M; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > } > > static const struct drm_connector_funcs vc4_vec_connector_funcs = { > @@ -297,15 +390,19 @@ static const struct drm_connector_funcs vc4_vec_connector_funcs = { > .reset = vc4_vec_connector_reset, > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > + .atomic_get_property = vc4_vec_connector_get_property, > + .atomic_set_property = vc4_vec_connector_set_property, > }; > > static const struct drm_connector_helper_funcs vc4_vec_connector_helper_funcs = { > + .atomic_check = drm_atomic_helper_connector_tv_check, > .get_modes = vc4_vec_connector_get_modes, > }; > > static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec) > { > struct drm_connector *connector = &vec->connector; > + struct drm_property *prop; > int ret; > > connector->interlace_allowed = true; > @@ -318,8 +415,17 @@ static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec) > drm_connector_helper_add(connector, &vc4_vec_connector_helper_funcs); > > drm_object_attach_property(&connector->base, > - dev->mode_config.legacy_tv_mode_property, > - VC4_VEC_TV_MODE_NTSC); > + dev->mode_config.tv_mode_property, > + DRM_MODE_TV_MODE_NTSC); > + > + prop = drm_property_create_enum(dev, 0, "mode", > + legacy_tv_mode_names, > + ARRAY_SIZE(legacy_tv_mode_names)); > + if (!prop) > + return -ENOMEM; > + vec->legacy_tv_mode_property = prop; > + > + drm_object_attach_property(&connector->base, prop, VC4_VEC_TV_MODE_NTSC); > > drm_connector_attach_encoder(connector, &vec->encoder.base); > > @@ -366,13 +472,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder, > struct drm_connector *connector = &vec->connector; > struct drm_connector_state *conn_state = > drm_atomic_get_new_connector_state(state, connector); > - const struct vc4_vec_tv_mode *tv_mode = > - &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > + const struct vc4_vec_tv_mode *tv_mode; > int idx, ret; > > if (!drm_dev_enter(drm, &idx)) > return; > > + tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode); > + if (!tv_mode) > + goto err_dev_exit; > + > ret = pm_runtime_get_sync(&vec->pdev->dev); > if (ret < 0) { > DRM_ERROR("Failed to retain power domain: %d\n", ret); > @@ -454,13 +563,6 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > struct drm_connector_state *conn_state) > { > const struct drm_display_mode *mode = &crtc_state->adjusted_mode; > - const struct vc4_vec_tv_mode *vec_mode; > - > - vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > - > - if (conn_state->crtc && > - !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > - return -EINVAL; > > if (mode->crtc_hdisplay % 4) > return -EINVAL; > @@ -554,13 +656,6 @@ static const struct of_device_id vc4_vec_dt_match[] = { > { /* sentinel */ }, > }; > > -static const char * const tv_mode_names[] = { > - [VC4_VEC_TV_MODE_NTSC] = "NTSC", > - [VC4_VEC_TV_MODE_NTSC_J] = "NTSC-J", > - [VC4_VEC_TV_MODE_PAL] = "PAL", > - [VC4_VEC_TV_MODE_PAL_M] = "PAL-M", > -}; > - > static int vc4_vec_bind(struct device *dev, struct device *master, void *data) > { > struct platform_device *pdev = to_platform_device(dev); > @@ -568,9 +663,11 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) > struct vc4_vec *vec; > int ret; > > - ret = drm_mode_create_tv_properties_legacy(drm, > - ARRAY_SIZE(tv_mode_names), > - tv_mode_names); > + ret = drm_mode_create_tv_properties(drm, > + BIT(DRM_MODE_TV_MODE_NTSC) | > + BIT(DRM_MODE_TV_MODE_NTSC_J) | > + BIT(DRM_MODE_TV_MODE_PAL) | > + BIT(DRM_MODE_TV_MODE_PAL_M)); > if (ret) > return ret; > >
On Mon, Oct 17, 2022 at 12:31:31PM +0200, Noralf Trønnes wrote: > Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski: > >> static int vc4_vec_connector_get_modes(struct drm_connector *connector) > >> { > >> - struct drm_connector_state *state = connector->state; > >> struct drm_display_mode *mode; > >> > >> - mode = drm_mode_duplicate(connector->dev, > >> - vc4_vec_tv_modes[state->tv.legacy_mode].mode); > >> + mode = drm_mode_analog_ntsc_480i(connector->dev); > >> if (!mode) { > >> DRM_ERROR("Failed to create a new display mode\n"); > >> return -ENOMEM; > >> } > >> > >> + mode->type |= DRM_MODE_TYPE_PREFERRED; > >> drm_mode_probed_add(connector, mode); > >> > >> - return 1; > >> + mode = drm_mode_analog_pal_576i(connector->dev); > >> + if (!mode) { > >> + DRM_ERROR("Failed to create a new display mode\n"); > >> + return -ENOMEM; > >> + } > >> + > >> + drm_mode_probed_add(connector, mode); > >> + > >> + return 2; > >> +} > > > > Referencing those previous discussions: > > - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/ > > - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/ > > > > Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg > > (at least on current Raspberry Pi OS) to display garbage when > > video=Composite1:PAL is specified on the command line, so I'm afraid this won't > > do. > > > > As I see it, there are three viable solutions for this issue: > > > > a) Somehow query the video= command line option from this function, and set > > DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction > > provided by global DRM code, but should work fine. > > > > b) Modify drm_helper_probe_add_cmdline_mode() so that it sets > > DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems > > pretty robust, but affects the entire DRM subsystem, which may break > > userspace in different ways. > > > > - Maybe this could be mitigated by adding some additional conditions, e.g. > > setting the PREFERRED flag only if no modes are already flagged as such > > and/or only if the cmdline mode is a named one (~= analog TV mode) > > > > c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF > > flag. > > > > Either way, hardcoding 480i as PREFERRED does not seem right. > > > > My solution for this is to look at tv.mode to know which mode to mark as > preferred. Maxime didn't like this since it changes things behind > userspace's back. I don't see how that can cause any problems for userspace. > > If userspace uses atomic and sets tv_mode, it has to know which mode to > use before hand, so it doesn't look at the preferreded flag. > > If it uses legacy and sets tv_mode, it can end up with a stale preferred > flag, but no worse than not having the flag or that ntsc is always > preferred. > > If it doesn't change tv_mode, there's no problem, the preferred flag > doesn't change. I don't like it because I just see no way to make this reliable. When we set tv_mode, we're not only going to change the preferred flag, but also the order of the modes to make the preferred mode first. Since we just changed the mode lists, we also want to send a hotplug event to userspace so that it gets notified of it. It will pick up the new preferred mode, great. But what if it doesn't? There's no requirement for userspace to handle hotplug events, and Kodi won't for example. So we just changed the TV mode but not the actual mode, and that's it. It's just as broken for Kodi as it is for X11 right now. If we can't get a bullet-proof solution, then I'm not convinced it's worth addressing. Especially since it's already the current state, and it doesn't seem to bother a lot of people. Maxime
Hi Maxime, W dniu 18.10.2022 o 12:00, Maxime Ripard pisze: > On Mon, Oct 17, 2022 at 12:31:31PM +0200, Noralf Trønnes wrote: >> Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski: >>>> static int vc4_vec_connector_get_modes(struct drm_connector *connector) >>>> { >>>> - struct drm_connector_state *state = connector->state; >>>> struct drm_display_mode *mode; >>>> >>>> - mode = drm_mode_duplicate(connector->dev, >>>> - vc4_vec_tv_modes[state->tv.legacy_mode].mode); >>>> + mode = drm_mode_analog_ntsc_480i(connector->dev); >>>> if (!mode) { >>>> DRM_ERROR("Failed to create a new display mode\n"); >>>> return -ENOMEM; >>>> } >>>> >>>> + mode->type |= DRM_MODE_TYPE_PREFERRED; >>>> drm_mode_probed_add(connector, mode); >>>> >>>> - return 1; >>>> + mode = drm_mode_analog_pal_576i(connector->dev); >>>> + if (!mode) { >>>> + DRM_ERROR("Failed to create a new display mode\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + drm_mode_probed_add(connector, mode); >>>> + >>>> + return 2; >>>> +} >>> >>> Referencing those previous discussions: >>> - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/ >>> - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/ >>> >>> Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg >>> (at least on current Raspberry Pi OS) to display garbage when >>> video=Composite1:PAL is specified on the command line, so I'm afraid this won't >>> do. >>> >>> As I see it, there are three viable solutions for this issue: >>> >>> a) Somehow query the video= command line option from this function, and set >>> DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction >>> provided by global DRM code, but should work fine. >>> >>> b) Modify drm_helper_probe_add_cmdline_mode() so that it sets >>> DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems >>> pretty robust, but affects the entire DRM subsystem, which may break >>> userspace in different ways. >>> >>> - Maybe this could be mitigated by adding some additional conditions, e.g. >>> setting the PREFERRED flag only if no modes are already flagged as such >>> and/or only if the cmdline mode is a named one (~= analog TV mode) >>> >>> c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF >>> flag. >>> >>> Either way, hardcoding 480i as PREFERRED does not seem right. >>> >> >> My solution for this is to look at tv.mode to know which mode to mark as >> preferred. Maxime didn't like this since it changes things behind >> userspace's back. I don't see how that can cause any problems for userspace. >> >> If userspace uses atomic and sets tv_mode, it has to know which mode to >> use before hand, so it doesn't look at the preferreded flag. >> >> If it uses legacy and sets tv_mode, it can end up with a stale preferred >> flag, but no worse than not having the flag or that ntsc is always >> preferred. >> >> If it doesn't change tv_mode, there's no problem, the preferred flag >> doesn't change. > > I don't like it because I just see no way to make this reliable. When we > set tv_mode, we're not only going to change the preferred flag, but also > the order of the modes to make the preferred mode first. > > Since we just changed the mode lists, we also want to send a hotplug > event to userspace so that it gets notified of it. It will pick up the > new preferred mode, great. > > But what if it doesn't? There's no requirement for userspace to handle > hotplug events, and Kodi won't for example. So we just changed the TV > mode but not the actual mode, and that's it. It's just as broken for > Kodi as it is for X11 right now. > > If we can't get a bullet-proof solution, then I'm not convinced it's > worth addressing. Especially since it's already the current state, and > it doesn't seem to bother a lot of people. I wouldn't rely on the "doesn't seem to bother a lot of people" bit too much. Here's why: - Analog TV output is a relatively obscure feature in this day and age in the first place. - Out of the people interested in using it with VC4/VEC, many are actually using the downstream kernel from https://github.com/raspberrypi/linux instead of the upstream kernel, and/or firmware mode-switching instead of proper KMS. - The downstream kernel only reports modes that match the TV mode set at boot either via vc4.tv_norm=, or implied by the resolution set via video=; note that video= is also set appropriately at boot by Pi firmware, based on the value of sdtv_mode set in config.txt. See also the vc4_vec_connector_get_modes() and vc4_vec_get_default_mode() functions in https://github.com/raspberrypi/linux/blob/dbd073e4028580a09b6ee507e0c137441cb52650/drivers/gpu/drm/vc4/vc4_vec.c - When firmware mode-switching is used, it sets the appropriate TV mode and resolution based on the sdtv_mode set in config.txt. So, all in all, the number of people who would use 1. analog TV out with VC4, 2. the upstream kernel, 3. full KMS (and thus the vc4_vec.c code) is rather small, so the fact that you're not hearing too many complaints doesn't mean that the current behavior is OK. If anybody ran into problems and was bothered by that, they likely migrated to the downstream kernel and/or firmware mode-switching. That being said, I completely forgot that there's a cmdline_mode field in struct drm_connector, even though I actually added code that examines it inside vc4_vec_connector_get_modes() that's in the downstream kernel. So... what do you think about just examining connector->cmdline_mode.tv_mode there? It seems to solve all the problems. Best regards, Mateusz Kwiatkowski
Hi, On Tue, Oct 18, 2022 at 11:28:35PM +0200, Mateusz Kwiatkowski wrote: > W dniu 18.10.2022 o 12:00, Maxime Ripard pisze: > > On Mon, Oct 17, 2022 at 12:31:31PM +0200, Noralf Trønnes wrote: > >> Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski: > >>>> static int vc4_vec_connector_get_modes(struct drm_connector *connector) > >>>> { > >>>> - struct drm_connector_state *state = connector->state; > >>>> struct drm_display_mode *mode; > >>>> > >>>> - mode = drm_mode_duplicate(connector->dev, > >>>> - vc4_vec_tv_modes[state->tv.legacy_mode].mode); > >>>> + mode = drm_mode_analog_ntsc_480i(connector->dev); > >>>> if (!mode) { > >>>> DRM_ERROR("Failed to create a new display mode\n"); > >>>> return -ENOMEM; > >>>> } > >>>> > >>>> + mode->type |= DRM_MODE_TYPE_PREFERRED; > >>>> drm_mode_probed_add(connector, mode); > >>>> > >>>> - return 1; > >>>> + mode = drm_mode_analog_pal_576i(connector->dev); > >>>> + if (!mode) { > >>>> + DRM_ERROR("Failed to create a new display mode\n"); > >>>> + return -ENOMEM; > >>>> + } > >>>> + > >>>> + drm_mode_probed_add(connector, mode); > >>>> + > >>>> + return 2; > >>>> +} > >>> > >>> Referencing those previous discussions: > >>> - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee5d6@tronnes.org/ > >>> - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cacd16@gmail.com/ > >>> > >>> Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg > >>> (at least on current Raspberry Pi OS) to display garbage when > >>> video=Composite1:PAL is specified on the command line, so I'm afraid this won't > >>> do. > >>> > >>> As I see it, there are three viable solutions for this issue: > >>> > >>> a) Somehow query the video= command line option from this function, and set > >>> DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction > >>> provided by global DRM code, but should work fine. > >>> > >>> b) Modify drm_helper_probe_add_cmdline_mode() so that it sets > >>> DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems > >>> pretty robust, but affects the entire DRM subsystem, which may break > >>> userspace in different ways. > >>> > >>> - Maybe this could be mitigated by adding some additional conditions, e.g. > >>> setting the PREFERRED flag only if no modes are already flagged as such > >>> and/or only if the cmdline mode is a named one (~= analog TV mode) > >>> > >>> c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF > >>> flag. > >>> > >>> Either way, hardcoding 480i as PREFERRED does not seem right. > >>> > >> > >> My solution for this is to look at tv.mode to know which mode to mark as > >> preferred. Maxime didn't like this since it changes things behind > >> userspace's back. I don't see how that can cause any problems for userspace. > >> > >> If userspace uses atomic and sets tv_mode, it has to know which mode to > >> use before hand, so it doesn't look at the preferreded flag. > >> > >> If it uses legacy and sets tv_mode, it can end up with a stale preferred > >> flag, but no worse than not having the flag or that ntsc is always > >> preferred. > >> > >> If it doesn't change tv_mode, there's no problem, the preferred flag > >> doesn't change. > > > > I don't like it because I just see no way to make this reliable. When we > > set tv_mode, we're not only going to change the preferred flag, but also > > the order of the modes to make the preferred mode first. > > > > Since we just changed the mode lists, we also want to send a hotplug > > event to userspace so that it gets notified of it. It will pick up the > > new preferred mode, great. > > > > But what if it doesn't? There's no requirement for userspace to handle > > hotplug events, and Kodi won't for example. So we just changed the TV > > mode but not the actual mode, and that's it. It's just as broken for > > Kodi as it is for X11 right now. > > > > If we can't get a bullet-proof solution, then I'm not convinced it's > > worth addressing. Especially since it's already the current state, and > > it doesn't seem to bother a lot of people. > > I wouldn't rely on the "doesn't seem to bother a lot of people" bit too much. > Here's why: > > - Analog TV output is a relatively obscure feature in this day and age in the > first place. > > - Out of the people interested in using it with VC4/VEC, many are actually using > the downstream kernel from https://github.com/raspberrypi/linux instead of the > upstream kernel, and/or firmware mode-switching instead of proper KMS. > > - The downstream kernel only reports modes that match the TV mode set at boot > either via vc4.tv_norm=, or implied by the resolution set via video=; note > that video= is also set appropriately at boot by Pi firmware, based on the > value of sdtv_mode set in config.txt. See also the > vc4_vec_connector_get_modes() and vc4_vec_get_default_mode() functions in > https://github.com/raspberrypi/linux/blob/dbd073e4028580a09b6ee507e0c137441cb52650/drivers/gpu/drm/vc4/vc4_vec.c > > - When firmware mode-switching is used, it sets the appropriate TV mode and > resolution based on the sdtv_mode set in config.txt. > > So, all in all, the number of people who would use 1. analog TV out with VC4, > 2. the upstream kernel, 3. full KMS (and thus the vc4_vec.c code) is rather > small, so the fact that you're not hearing too many complaints doesn't mean that > the current behavior is OK. If anybody ran into problems and was bothered by > that, they likely migrated to the downstream kernel and/or firmware > mode-switching. > > That being said, I completely forgot that there's a cmdline_mode field in > struct drm_connector, even though I actually added code that examines it inside > vc4_vec_connector_get_modes() that's in the downstream kernel. So... what do > you think about just examining connector->cmdline_mode.tv_mode there? It seems > to solve all the problems. It's a very good idea, I'll work on it, thanks :) Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index 1fcb7baf874e..456689b45849 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -172,6 +172,8 @@ struct vc4_vec { struct clk *clock; + struct drm_property *legacy_tv_mode_property; + struct debugfs_regset32 regset; }; @@ -184,6 +186,12 @@ encoder_to_vc4_vec(struct drm_encoder *encoder) return container_of(encoder, struct vc4_vec, encoder.base); } +static inline struct vc4_vec * +connector_to_vc4_vec(struct drm_connector *connector) +{ + return container_of(connector, struct vc4_vec, connector); +} + enum vc4_vec_tv_mode_id { VC4_VEC_TV_MODE_NTSC, VC4_VEC_TV_MODE_NTSC_J, @@ -192,7 +200,7 @@ enum vc4_vec_tv_mode_id { }; struct vc4_vec_tv_mode { - const struct drm_display_mode *mode; + unsigned int mode; u32 config0; u32 config1; u32 custom_freq; @@ -225,43 +233,51 @@ static const struct debugfs_reg32 vec_regs[] = { VC4_REG32(VEC_DAC_MISC), }; -static const struct drm_display_mode ntsc_mode = { - DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 13500, - 720, 720 + 14, 720 + 14 + 64, 720 + 14 + 64 + 60, 0, - 480, 480 + 7, 480 + 7 + 6, 525, 0, - DRM_MODE_FLAG_INTERLACE) -}; - -static const struct drm_display_mode pal_mode = { - DRM_MODE("720x576", DRM_MODE_TYPE_DRIVER, 13500, - 720, 720 + 20, 720 + 20 + 64, 720 + 20 + 64 + 60, 0, - 576, 576 + 4, 576 + 4 + 6, 625, 0, - DRM_MODE_FLAG_INTERLACE) -}; - static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = { - [VC4_VEC_TV_MODE_NTSC] = { - .mode = &ntsc_mode, + { + .mode = DRM_MODE_TV_MODE_NTSC, .config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, - [VC4_VEC_TV_MODE_NTSC_J] = { - .mode = &ntsc_mode, + { + .mode = DRM_MODE_TV_MODE_NTSC_J, .config0 = VEC_CONFIG0_NTSC_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, - [VC4_VEC_TV_MODE_PAL] = { - .mode = &pal_mode, + { + .mode = DRM_MODE_TV_MODE_PAL, .config0 = VEC_CONFIG0_PAL_BDGHI_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, - [VC4_VEC_TV_MODE_PAL_M] = { - .mode = &ntsc_mode, + { + .mode = DRM_MODE_TV_MODE_PAL_M, .config0 = VEC_CONFIG0_PAL_M_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, }; +static inline const struct vc4_vec_tv_mode * +vc4_vec_tv_mode_lookup(unsigned int mode) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) { + const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i]; + + if (tv_mode->mode == mode) + return tv_mode; + } + + return NULL; +} + +static const struct drm_prop_enum_list legacy_tv_mode_names[] = { + { VC4_VEC_TV_MODE_NTSC, "NTSC", }, + { VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", }, + { VC4_VEC_TV_MODE_PAL, "PAL", }, + { VC4_VEC_TV_MODE_PAL_M, "PAL-M", }, +}; + static enum drm_connector_status vc4_vec_connector_detect(struct drm_connector *connector, bool force) { @@ -276,19 +292,96 @@ static void vc4_vec_connector_reset(struct drm_connector *connector) static int vc4_vec_connector_get_modes(struct drm_connector *connector) { - struct drm_connector_state *state = connector->state; struct drm_display_mode *mode; - mode = drm_mode_duplicate(connector->dev, - vc4_vec_tv_modes[state->tv.legacy_mode].mode); + mode = drm_mode_analog_ntsc_480i(connector->dev); if (!mode) { DRM_ERROR("Failed to create a new display mode\n"); return -ENOMEM; } + mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); - return 1; + mode = drm_mode_analog_pal_576i(connector->dev); + if (!mode) { + DRM_ERROR("Failed to create a new display mode\n"); + return -ENOMEM; + } + + drm_mode_probed_add(connector, mode); + + return 2; +} + +static int +vc4_vec_connector_set_property(struct drm_connector *connector, + struct drm_connector_state *state, + struct drm_property *property, + uint64_t val) +{ + struct vc4_vec *vec = connector_to_vc4_vec(connector); + + if (property != vec->legacy_tv_mode_property) + return -EINVAL; + + switch (val) { + case VC4_VEC_TV_MODE_NTSC: + state->tv.mode = DRM_MODE_TV_MODE_NTSC; + break; + + case VC4_VEC_TV_MODE_NTSC_J: + state->tv.mode = DRM_MODE_TV_MODE_NTSC_J; + break; + + case VC4_VEC_TV_MODE_PAL: + state->tv.mode = DRM_MODE_TV_MODE_PAL; + break; + + case VC4_VEC_TV_MODE_PAL_M: + state->tv.mode = DRM_MODE_TV_MODE_PAL_M; + break; + + default: + return -EINVAL; + } + + return 0; +} + +static int +vc4_vec_connector_get_property(struct drm_connector *connector, + const struct drm_connector_state *state, + struct drm_property *property, + uint64_t *val) +{ + struct vc4_vec *vec = connector_to_vc4_vec(connector); + + if (property != vec->legacy_tv_mode_property) + return -EINVAL; + + switch (state->tv.mode) { + case DRM_MODE_TV_MODE_NTSC: + *val = VC4_VEC_TV_MODE_NTSC; + break; + + case DRM_MODE_TV_MODE_NTSC_J: + *val = VC4_VEC_TV_MODE_NTSC_J; + break; + + case DRM_MODE_TV_MODE_PAL: + *val = VC4_VEC_TV_MODE_PAL; + break; + + case DRM_MODE_TV_MODE_PAL_M: + *val = VC4_VEC_TV_MODE_PAL_M; + break; + + default: + return -EINVAL; + } + + return 0; } static const struct drm_connector_funcs vc4_vec_connector_funcs = { @@ -297,15 +390,19 @@ static const struct drm_connector_funcs vc4_vec_connector_funcs = { .reset = vc4_vec_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, + .atomic_get_property = vc4_vec_connector_get_property, + .atomic_set_property = vc4_vec_connector_set_property, }; static const struct drm_connector_helper_funcs vc4_vec_connector_helper_funcs = { + .atomic_check = drm_atomic_helper_connector_tv_check, .get_modes = vc4_vec_connector_get_modes, }; static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec) { struct drm_connector *connector = &vec->connector; + struct drm_property *prop; int ret; connector->interlace_allowed = true; @@ -318,8 +415,17 @@ static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec) drm_connector_helper_add(connector, &vc4_vec_connector_helper_funcs); drm_object_attach_property(&connector->base, - dev->mode_config.legacy_tv_mode_property, - VC4_VEC_TV_MODE_NTSC); + dev->mode_config.tv_mode_property, + DRM_MODE_TV_MODE_NTSC); + + prop = drm_property_create_enum(dev, 0, "mode", + legacy_tv_mode_names, + ARRAY_SIZE(legacy_tv_mode_names)); + if (!prop) + return -ENOMEM; + vec->legacy_tv_mode_property = prop; + + drm_object_attach_property(&connector->base, prop, VC4_VEC_TV_MODE_NTSC); drm_connector_attach_encoder(connector, &vec->encoder.base); @@ -366,13 +472,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder, struct drm_connector *connector = &vec->connector; struct drm_connector_state *conn_state = drm_atomic_get_new_connector_state(state, connector); - const struct vc4_vec_tv_mode *tv_mode = - &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; + const struct vc4_vec_tv_mode *tv_mode; int idx, ret; if (!drm_dev_enter(drm, &idx)) return; + tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode); + if (!tv_mode) + goto err_dev_exit; + ret = pm_runtime_get_sync(&vec->pdev->dev); if (ret < 0) { DRM_ERROR("Failed to retain power domain: %d\n", ret); @@ -454,13 +563,6 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, struct drm_connector_state *conn_state) { const struct drm_display_mode *mode = &crtc_state->adjusted_mode; - const struct vc4_vec_tv_mode *vec_mode; - - vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; - - if (conn_state->crtc && - !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) - return -EINVAL; if (mode->crtc_hdisplay % 4) return -EINVAL; @@ -554,13 +656,6 @@ static const struct of_device_id vc4_vec_dt_match[] = { { /* sentinel */ }, }; -static const char * const tv_mode_names[] = { - [VC4_VEC_TV_MODE_NTSC] = "NTSC", - [VC4_VEC_TV_MODE_NTSC_J] = "NTSC-J", - [VC4_VEC_TV_MODE_PAL] = "PAL", - [VC4_VEC_TV_MODE_PAL_M] = "PAL-M", -}; - static int vc4_vec_bind(struct device *dev, struct device *master, void *data) { struct platform_device *pdev = to_platform_device(dev); @@ -568,9 +663,11 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) struct vc4_vec *vec; int ret; - ret = drm_mode_create_tv_properties_legacy(drm, - ARRAY_SIZE(tv_mode_names), - tv_mode_names); + ret = drm_mode_create_tv_properties(drm, + BIT(DRM_MODE_TV_MODE_NTSC) | + BIT(DRM_MODE_TV_MODE_NTSC_J) | + BIT(DRM_MODE_TV_MODE_PAL) | + BIT(DRM_MODE_TV_MODE_PAL_M)); if (ret) return ret;