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.
Signed-off-by: Maxime Ripard <[email protected]>
diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index ba6f81908923..58286acf4b9e 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;
@@ -226,28 +234,50 @@ static const struct debugfs_reg32 vec_regs[] = {
};
static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
- [VC4_VEC_TV_MODE_NTSC] = {
- .mode = &drm_mode_480i,
+ {
+ .mode = DRM_MODE_TV_MODE_NTSC_M,
.config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
.config1 = VEC_CONFIG1_C_CVBS_CVBS,
},
- [VC4_VEC_TV_MODE_NTSC_J] = {
- .mode = &drm_mode_480i,
+ {
+ .mode = DRM_MODE_TV_MODE_NTSC_J,
.config0 = VEC_CONFIG0_NTSC_STD,
.config1 = VEC_CONFIG1_C_CVBS_CVBS,
},
- [VC4_VEC_TV_MODE_PAL] = {
- .mode = &drm_mode_576i,
+ {
+ .mode = DRM_MODE_TV_MODE_PAL_B,
.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
.config1 = VEC_CONFIG1_C_CVBS_CVBS,
},
- [VC4_VEC_TV_MODE_PAL_M] = {
- .mode = &drm_mode_480i,
+ {
+ .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 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)
{
@@ -262,19 +292,98 @@ 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;
+ 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;
+}
+
+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_M;
+ 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_B;
+ 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_J:
+ *val = VC4_VEC_TV_MODE_NTSC_J;
+ break;
+
+ case DRM_MODE_TV_MODE_NTSC_M:
+ *val = VC4_VEC_TV_MODE_NTSC;
+ break;
+
+ case DRM_MODE_TV_MODE_PAL_B:
+ *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 = {
@@ -283,15 +392,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;
@@ -304,8 +417,16 @@ 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_M);
+
+ prop = drm_property_create_enum(dev, 0, "mode",
+ tv_mode_names, ARRAY_SIZE(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);
@@ -352,13 +473,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.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);
@@ -435,23 +559,7 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
drm_dev_exit(idx);
}
-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;
-}
-
static const struct drm_encoder_helper_funcs vc4_vec_encoder_helper_funcs = {
- .atomic_check = vc4_vec_encoder_atomic_check,
.atomic_disable = vc4_vec_encoder_disable,
.atomic_enable = vc4_vec_encoder_enable,
};
@@ -492,13 +600,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);
@@ -506,9 +607,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_J) |
+ BIT(DRM_MODE_TV_MODE_NTSC_M) |
+ BIT(DRM_MODE_TV_MODE_PAL_B) |
+ BIT(DRM_MODE_TV_MODE_PAL_M));
if (ret)
return ret;
--
b4 0.10.0-dev-65ba7
Den 29.08.2022 15.11, 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.
>
>
>
> Signed-off-by: Maxime Ripard <[email protected]>
>
>
>
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
>
> index ba6f81908923..58286acf4b9e 100644
>
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
>
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -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;
>
> @@ -226,28 +234,50 @@ static const struct debugfs_reg32 vec_regs[] = {
>
> };
>
>
>
> static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>
> - [VC4_VEC_TV_MODE_NTSC] = {
>
> - .mode = &drm_mode_480i,
>
> + {
>
> + .mode = DRM_MODE_TV_MODE_NTSC_M,
>
> .config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> - [VC4_VEC_TV_MODE_NTSC_J] = {
>
> - .mode = &drm_mode_480i,
>
> + {
>
> + .mode = DRM_MODE_TV_MODE_NTSC_J,
>
> .config0 = VEC_CONFIG0_NTSC_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> - [VC4_VEC_TV_MODE_PAL] = {
>
> - .mode = &drm_mode_576i,
>
> + {
>
> + .mode = DRM_MODE_TV_MODE_PAL_B,
>
> .config0 = VEC_CONFIG0_PAL_BDGHI_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> - [VC4_VEC_TV_MODE_PAL_M] = {
>
> - .mode = &drm_mode_480i,
>
> + {
>
> + .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 tv_mode_names[] = {
Maybe call it legacy_tv_mode_enums?
>
> + { 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", },
If you use DRM_MODE_TV_MODE_* here you don't need to translate the value
using the switch statement in get/set property, you can use the value
directly to get/set tv.mode.
Noralf.
>
> +};
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
Hi Maxime,
W dniu 08.09.2022 o 13:23, Maxime Ripard pisze:
> Hi Noralf,
>
> On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Trønnes wrote:
>>> +static const struct drm_prop_enum_list tv_mode_names[] = {
>>
>> Maybe call it legacy_tv_mode_enums?
>>
>>>
>>> + { 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", },
>>
>> If you use DRM_MODE_TV_MODE_* here you don't need to translate the value
>> using the switch statement in get/set property, you can use the value
>> directly to get/set tv.mode.
>
> I'm sorry, I'm not quite sure what you mean by that. If we expose the
> DRM_MODE_TV_MODE_* properties there, won't that change the values the
> userspace will need to use to set that property?
I'd just like to point out that if numerical values of these enums are your
concern, then you're (or perhaps I am ;) already breaking this by adding new
modes in patch 33/41 in this series.
And the values (and names!) added by that patch (33/41) don't match those
currently used by the downstream version
(https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/gpu/drm/vc4/vc4_vec.c).
If any userspace software is manipulating this property, it's most likely
targeting the downstream code. But since you're not aiming for consistency with
that, I was under the impression that compatibility isn't a concern.
Best regards,
Mateusz Kwiatkowski
Den 08.09.2022 13.23, skrev Maxime Ripard:
> Hi Noralf,
>
> On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Trønnes wrote:
>>> +static const struct drm_prop_enum_list tv_mode_names[] = {
>>
>> Maybe call it legacy_tv_mode_enums?
>>
>>>
>>> + { 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", },
>>
>> If you use DRM_MODE_TV_MODE_* here you don't need to translate the value
>> using the switch statement in get/set property, you can use the value
>> directly to get/set tv.mode.
>
> I'm sorry, I'm not quite sure what you mean by that. If we expose the
> DRM_MODE_TV_MODE_* properties there, won't that change the values the
> userspace will need to use to set that property?
>
You're right ofc, I forgot that the enum values are also UABI.
Noralf.
Hi Noralf,
On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Tr?nnes wrote:
> > +static const struct drm_prop_enum_list tv_mode_names[] = {
>
> Maybe call it legacy_tv_mode_enums?
>
> >
> > + { 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", },
>
> If you use DRM_MODE_TV_MODE_* here you don't need to translate the value
> using the switch statement in get/set property, you can use the value
> directly to get/set tv.mode.
I'm sorry, I'm not quite sure what you mean by that. If we expose the
DRM_MODE_TV_MODE_* properties there, won't that change the values the
userspace will need to use to set that property?
Maxime
On Thu, Sep 08, 2022 at 01:31:34PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 08.09.2022 o 13:23, Maxime Ripard pisze:
> > Hi Noralf,
> >
> > On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Tr?nnes wrote:
> >>> +static const struct drm_prop_enum_list tv_mode_names[] = {
> >>
> >> Maybe call it legacy_tv_mode_enums?
> >>
> >>>
> >>> +?? ?{ 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", },
> >>
> >> If you use DRM_MODE_TV_MODE_* here you don't need to translate the value
> >> using the switch statement in get/set property, you can use the value
> >> directly to get/set tv.mode.
> >
> > I'm sorry, I'm not quite sure what you mean by that. If we expose the
> > DRM_MODE_TV_MODE_* properties there, won't that change the values the
> > userspace will need to use to set that property?
>
> I'd just like to point out that if numerical values of these enums are your
> concern, then you're (or perhaps I am ;) already breaking this by adding new
> modes in patch 33/41 in this series.
>
> And the values (and names!) added by that patch (33/41) don't match those
> currently used by the downstream version
> (https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/gpu/drm/vc4/vc4_vec.c).
> If any userspace software is manipulating this property, it's most likely
> targeting the downstream code. But since you're not aiming for consistency with
> that, I was under the impression that compatibility isn't a concern.
I'm not really concerned about the compatibility with the downstream
tree, if only because you already broke that compatibility with your
patch :)
So you're right, I'll reorganize that patch to keep the backward
compatibility.
Maxime
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
