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/[email protected]/
- https://lore.kernel.org/dri-devel/[email protected]/
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
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/[email protected]/
> - https://lore.kernel.org/dri-devel/[email protected]/
>
> 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.
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/[email protected]/
> > - https://lore.kernel.org/dri-devel/[email protected]/
> >
> > 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