Nouveau - Sep 2022 - [PATCH v2 32/41] drm/vc4: vec: Convert to the new TV mode property

Hi Maxime,

I tested your patchset on my Pi and it mostly works. Good work! However,
I noticed a couple of issues.
> -static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
> -?? ??? ??? ??? ??? ?struct drm_crtc_state *crtc_state,
> -?? ??? ??? ??? ??? ?struct drm_connector_state *conn_state)
> -{
> -?? ?const struct vc4_vec_tv_mode *vec_mode;
> -
> -?? ?vec_mode = &vc4_vec_tv_modes[conn_state->tv.mode];
> -
> -?? ?if (conn_state->crtc &&
> -?? ???? !drm_mode_equal(vec_mode->mode,
&crtc_state->adjusted_mode))
> -?? ??? ?return -EINVAL;
> -
> -?? ?return 0;
> -}
I may have said it myself that we should allow custom modelines without too
much validation. The VC4 and VEC, however, have some considerable limitations
when it comes to the modelines that they can reliably output.

In particular, attempting to use "50 Hz" timings in NTSC/PAL-M modes
(or
"60 Hz" in PAL/SECAM modes) results in a weirdly skewed image.
Here's how it
may look like:
https://user-images.githubusercontent.com/4499762/187575940-736e7262-c82d-42f3-a2d8-f309cbd51139.png

This is because if the CRTC does not trigger the sync pulses within an
acceptable time window, the VEC apparently generates them itself. This causes
the VEC sync pulses (which go onto the wire) not quite line up with the ones
from the modeline, which results in what you see on the screenshot.

I once wrote a validation function based on extensive testing of what
produces a sensible output and what doesn't. You can find it here:
https://github.com/raspberrypi/linux/pull/4406/commits/15c0c51. I think it
might be a good idea to include something like that - even though I know
it's
somewhat ugly.

(BTW, those %2 checks on vertical timings in that linked commit can be ignored;
those values are divided by 2 for interlaced modes anyway. Those checks were
intended to ensure proper odd-first or even-first timings; I'm not sure if
your
code calculates those in the same way)
>? static int vc4_vec_connector_get_modes(struct drm_connector *connector)
>? {
> -?? ?struct drm_connector_state *state = connector->state;
> ??? ?struct drm_display_mode *mode;
> +?? ?int count = 0;
> ?
> -?? ?mode = drm_mode_duplicate(connector->dev,
> -?? ??? ??? ??? ?? vc4_vec_tv_modes[state->tv.mode].mode);
> +?? ?mode = drm_mode_analog_ntsc_480i(connector->dev);
> ??? ?if (!mode) {
> ??? ??? ?DRM_ERROR("Failed to create a new display mode\n");
> ??? ??? ?return -ENOMEM;
> ??? ?}
> ?
> ??? ?drm_mode_probed_add(connector, mode);
> +?? ?count += 1;
> ?
> -?? ?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);
> +?? ?count += 1;
> +
> +?? ?return count;
> +}
Xorg is pretty confused by these modes being reported like that. The 576i mode
is *always* preferred, presumably because of the higher resolution. If the NTSC
mode is set (via the kernel cmdline or just due to it being the default), this
results in a mess on the screen - exactly the same thing as on the screenshot
linked above.

Note that drm_helper_probe_add_cmdline_mode() *does* add the
DRM_MODE_TYPE_USERDEF flag to the 480i mode, having detected it as preferred
on the command line - but Xorg does not seem to care about that.

I remember Noralf suggesting setting DRM_MODE_TYPE_PREFERRED for the mode that
corresponds to the currently chosen tv_mode - that would fix the problem.
An alternative would be to _not_ add the "opposite" mode at all, like
the
current default Raspberry Pi OS kernel behaves.

Note that if you decide to add the modeline validation like I suggested in the
comment above, then without setting the preferred mode properly, Xorg will just
give up and sit on a blank screen until you run xrandr from another terminal
if tv_mode incompatible with 576i is selected.

Best regards,
Mateusz Kwiatkowski

On Wed, Aug 31, 2022 at 04:23:21AM +0200, Mateusz Kwiatkowski
wrote:
> I tested your patchset on my Pi and it mostly works. Good work! However,
> I noticed a couple of issues.
> 
> > -static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
> > -?? ??? ??? ??? ??? ?struct drm_crtc_state *crtc_state,
> > -?? ??? ??? ??? ??? ?struct drm_connector_state *conn_state)
> > -{
> > -?? ?const struct vc4_vec_tv_mode *vec_mode;
> > -
> > -?? ?vec_mode = &vc4_vec_tv_modes[conn_state->tv.mode];
> > -
> > -?? ?if (conn_state->crtc &&
> > -?? ???? !drm_mode_equal(vec_mode->mode,
&crtc_state->adjusted_mode))
> > -?? ??? ?return -EINVAL;
> > -
> > -?? ?return 0;
> > -}
> 
> I may have said it myself that we should allow custom modelines without too
> much validation. The VC4 and VEC, however, have some considerable
limitations
> when it comes to the modelines that they can reliably output.
> 
> In particular, attempting to use "50 Hz" timings in NTSC/PAL-M
modes (or
> "60 Hz" in PAL/SECAM modes) results in a weirdly skewed image.
Here's how it
> may look like:
>
https://user-images.githubusercontent.com/4499762/187575940-736e7262-c82d-42f3-a2d8-f309cbd51139.png
> 
> This is because if the CRTC does not trigger the sync pulses within an
> acceptable time window, the VEC apparently generates them itself. This
causes
> the VEC sync pulses (which go onto the wire) not quite line up with the
ones
> from the modeline, which results in what you see on the screenshot.
> 
> I once wrote a validation function based on extensive testing of what
> produces a sensible output and what doesn't. You can find it here:
> https://github.com/raspberrypi/linux/pull/4406/commits/15c0c51. I think it
> might be a good idea to include something like that - even though I know
it's
> somewhat ugly.
I've reworked that code a bit, and it will be part of my next version.
> (BTW, those %2 checks on vertical timings in that linked commit can be
ignored;
> those values are divided by 2 for interlaced modes anyway. Those checks
were
> intended to ensure proper odd-first or even-first timings; I'm not sure
if your
> code calculates those in the same way)
Ack, I've removed them.
> >? static int vc4_vec_connector_get_modes(struct drm_connector
*connector)
> >? {
> > -?? ?struct drm_connector_state *state = connector->state;
> > ??? ?struct drm_display_mode *mode;
> > +?? ?int count = 0;
> > ?
> > -?? ?mode = drm_mode_duplicate(connector->dev,
> > -?? ??? ??? ??? ?? vc4_vec_tv_modes[state->tv.mode].mode);
> > +?? ?mode = drm_mode_analog_ntsc_480i(connector->dev);
> > ??? ?if (!mode) {
> > ??? ??? ?DRM_ERROR("Failed to create a new display mode\n");
> > ??? ??? ?return -ENOMEM;
> > ??? ?}
> > ?
> > ??? ?drm_mode_probed_add(connector, mode);
> > +?? ?count += 1;
> > ?
> > -?? ?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);
> > +?? ?count += 1;
> > +
> > +?? ?return count;
> > +}
> 
> Xorg is pretty confused by these modes being reported like that. The 576i
mode
> is *always* preferred, presumably because of the higher resolution. If the
NTSC
> mode is set (via the kernel cmdline or just due to it being the default),
this
> results in a mess on the screen - exactly the same thing as on the
screenshot
> linked above.
> 
> Note that drm_helper_probe_add_cmdline_mode() *does* add the
> DRM_MODE_TYPE_USERDEF flag to the 480i mode, having detected it as
preferred
> on the command line - but Xorg does not seem to care about that.
I'm not quite sure why that would be the case. The usual logic to pick
the preferred mode is to use either the mode with the flag or the first
one.
> I remember Noralf suggesting setting DRM_MODE_TYPE_PREFERRED for the mode
that
> corresponds to the currently chosen tv_mode - that would fix the problem.
> An alternative would be to _not_ add the "opposite" mode at all,
like the
> current default Raspberry Pi OS kernel behaves.
I'll add it the PREFERRED flag then, switching the modes have other
challenges.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220908/c5102b75/attachment.sig>