diff mbox series

[v1,23/35] drm/vc4: vec: Convert to the new TV mode property

Message ID 20220728-rpi-analog-tv-properties-v1-23-3d53ae722097@cerno.tech (mailing list archive)
State New, archived
Headers show
Series drm: Analog TV Improvements | expand

Commit Message

Maxime Ripard July 29, 2022, 4:35 p.m. UTC 
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 <maxime@cerno.tech>

Comments

Noralf Trønnes Aug. 20, 2022, 5:22 p.m. UTC | #1 
Den 29.07.2022 18.35, 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 <maxime@cerno.tech>
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c

>  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.mode].mode);
> +	mode = drm_mode_duplicate(connector->dev, &drm_mode_480i);
> +	if (!mode) {
> +		DRM_ERROR("Failed to create a new display mode\n");
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	mode = drm_mode_duplicate(connector->dev, &drm_mode_576i);

Maybe the mode that matches tv.norm should be marked as preferred so
userspace knows which one to pick?

Noralf.

>  	if (!mode) {
>  		DRM_ERROR("Failed to create a new display mode\n");
>  		return -ENOMEM;
> @@ -277,21 +313,95 @@ static int vc4_vec_connector_get_modes(struct drm_connector *connector)
>  	return 1;
>  }
Maxime Ripard Aug. 24, 2022, 3:26 p.m. UTC | #2 
Hi,

On Sat, Aug 20, 2022 at 07:22:48PM +0200, Noralf Trønnes wrote:
> Den 29.07.2022 18.35, 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 <maxime@cerno.tech>
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> 
> >  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.mode].mode);
> > +	mode = drm_mode_duplicate(connector->dev, &drm_mode_480i);
> > +	if (!mode) {
> > +		DRM_ERROR("Failed to create a new display mode\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	drm_mode_probed_add(connector, mode);
> > +
> > +	mode = drm_mode_duplicate(connector->dev, &drm_mode_576i);
> 
> Maybe the mode that matches tv.norm should be marked as preferred so
> userspace knows which one to pick?

I'm not sure how realistic that would be. Doing this based on the driver
/ cmdline preference is going to be fairly easy, but then it's a
property, it's going to be updated, and we probably don't want to mess
around the mode flags based on new property values?

Maxime
Noralf Trønnes Aug. 25, 2022, 1:14 p.m. UTC | #3 
Den 24.08.2022 17.26, skrev Maxime Ripard:
> Hi,
> 
> On Sat, Aug 20, 2022 at 07:22:48PM +0200, Noralf Trønnes wrote:
>> Den 29.07.2022 18.35, 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 <maxime@cerno.tech>
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
>>
>>>  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.mode].mode);
>>> +	mode = drm_mode_duplicate(connector->dev, &drm_mode_480i);
>>> +	if (!mode) {
>>> +		DRM_ERROR("Failed to create a new display mode\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	drm_mode_probed_add(connector, mode);
>>> +
>>> +	mode = drm_mode_duplicate(connector->dev, &drm_mode_576i);
>>
>> Maybe the mode that matches tv.norm should be marked as preferred so
>> userspace knows which one to pick?
> 
> I'm not sure how realistic that would be. Doing this based on the driver
> / cmdline preference is going to be fairly easy, but then it's a
> property, it's going to be updated, and we probably don't want to mess
> around the mode flags based on new property values?
> 

Strictly speaking we need to fire an event to userspace if the mode
changes, and this is probably not straightforward to do underneath
modeset locks, would probably need a worker.

Clever userspace like GNOME will try to use the active mode, so it will
handle this that way. If someone has set up the pipeline first that is.
drm_client/fbdev and plymouth can do that because they honour userdef modes.

Other userspace that don't know the userdef flag will fallback to the
first mode which is NTSC which is also the default tvmode, so maybe this
is good enough. PAL users will have to specify the mode, or teach their
program about the userdef flag.

But ofc relying on the userdef flag depends on the fact that there's a
mode on the kernel command line, but maybe there's no way to avoid that
since much/most? userspace treat "unknown" connector status as
disconnected so many will have to force the connector to "connected"
anyway. At least I don't know any way to permanetly force the connector
status other than using video=.

Noralf.
Maxime Ripard Aug. 29, 2022, 12:13 p.m. UTC | #4 
Hi Noralf,

On Thu, Aug 25, 2022 at 03:14:01PM +0200, Noralf Trønnes wrote:
> Den 24.08.2022 17.26, skrev Maxime Ripard:
> > On Sat, Aug 20, 2022 at 07:22:48PM +0200, Noralf Trønnes wrote:
> >> Den 29.07.2022 18.35, 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 <maxime@cerno.tech>
> >>>
> >>> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> >>
> >>>  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.mode].mode);
> >>> +	mode = drm_mode_duplicate(connector->dev, &drm_mode_480i);
> >>> +	if (!mode) {
> >>> +		DRM_ERROR("Failed to create a new display mode\n");
> >>> +		return -ENOMEM;
> >>> +	}
> >>> +
> >>> +	drm_mode_probed_add(connector, mode);
> >>> +
> >>> +	mode = drm_mode_duplicate(connector->dev, &drm_mode_576i);
> >>
> >> Maybe the mode that matches tv.norm should be marked as preferred so
> >> userspace knows which one to pick?
> > 
> > I'm not sure how realistic that would be. Doing this based on the driver
> > / cmdline preference is going to be fairly easy, but then it's a
> > property, it's going to be updated, and we probably don't want to mess
> > around the mode flags based on new property values?
> > 
> 
> Strictly speaking we need to fire an event to userspace if the mode
> changes, and this is probably not straightforward to do underneath
> modeset locks, would probably need a worker.

I'm not sure this would work in all cases. Kodi for example doesn't
handle hotplug events at all, so we might end up in situations where the
state is not consistent anymore.

Even if we were to only expose one mode to the userspace, depending on
the current TV mode, userspace could still end up trying to push a mode
into KMS that isn't the one we expose anymore, so I'm not sure we can
solve this entirely.

> Clever userspace like GNOME will try to use the active mode, so it will
> handle this that way. If someone has set up the pipeline first that is.
> drm_client/fbdev and plymouth can do that because they honour userdef modes.
> 
> Other userspace that don't know the userdef flag will fallback to the
> first mode which is NTSC which is also the default tvmode, so maybe this
> is good enough. PAL users will have to specify the mode, or teach their
> program about the userdef flag.
> 
> But ofc relying on the userdef flag depends on the fact that there's a
> mode on the kernel command line, but maybe there's no way to avoid that
> since much/most? userspace treat "unknown" connector status as
> disconnected so many will have to force the connector to "connected"
> anyway. At least I don't know any way to permanetly force the connector
> status other than using video=.

You can do it through sysfs as well, in .../status

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index 6f4536bf537f..e40b55de1b3c 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 *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_NORM_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_NORM_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_NORM_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_NORM_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,11 +292,17 @@  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.mode].mode);
+	mode = drm_mode_duplicate(connector->dev, &drm_mode_480i);
+	if (!mode) {
+		DRM_ERROR("Failed to create a new display mode\n");
+		return -ENOMEM;
+	}
+
+	drm_mode_probed_add(connector, mode);
+
+	mode = drm_mode_duplicate(connector->dev, &drm_mode_576i);
 	if (!mode) {
 		DRM_ERROR("Failed to create a new display mode\n");
 		return -ENOMEM;
@@ -277,21 +313,95 @@  static int vc4_vec_connector_get_modes(struct drm_connector *connector)
 	return 1;
 }
 
+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->tv_mode_property)
+		return -EINVAL;
+
+	switch (val) {
+	case VC4_VEC_TV_MODE_NTSC:
+		state->tv.norm = DRM_MODE_TV_NORM_NTSC_M;
+		break;
+
+	case VC4_VEC_TV_MODE_NTSC_J:
+		state->tv.norm = DRM_MODE_TV_NORM_NTSC_J;
+		break;
+
+	case VC4_VEC_TV_MODE_PAL:
+		state->tv.norm = DRM_MODE_TV_NORM_PAL_B;
+		break;
+
+	case VC4_VEC_TV_MODE_PAL_M:
+		state->tv.norm = DRM_MODE_TV_NORM_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->tv_mode_property)
+		return -EINVAL;
+
+	switch (state->tv.norm) {
+	case DRM_MODE_TV_NORM_NTSC_J:
+		*val = VC4_VEC_TV_MODE_NTSC_J;
+		break;
+
+	case DRM_MODE_TV_NORM_NTSC_M:
+		*val = VC4_VEC_TV_MODE_NTSC;
+		break;
+
+	case DRM_MODE_TV_NORM_PAL_B:
+		*val = VC4_VEC_TV_MODE_PAL;
+		break;
+
+	case DRM_MODE_TV_NORM_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 = {
 	.detect = vc4_vec_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.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 +414,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.tv_mode_property,
-				   VC4_VEC_TV_MODE_NTSC);
+				   dev->mode_config.tv_norm_property,
+				   DRM_MODE_TV_NORM_NTSC_M);
+
+	prop = drm_property_create_enum(dev, 0, "mode",
+					tv_mode_names, ARRAY_SIZE(tv_mode_names));
+	if (!prop)
+		return -ENOMEM;
+	vec->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 +470,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.norm);
+	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 +556,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 +597,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);
@@ -507,9 +605,11 @@  static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
 	int ret;
 
 	ret = drm_mode_create_tv_properties(drm,
-					    0,
-					    ARRAY_SIZE(tv_mode_names),
-					    tv_mode_names);
+					    DRM_MODE_TV_NORM_NTSC_J |
+					    DRM_MODE_TV_NORM_NTSC_M |
+					    DRM_MODE_TV_NORM_PAL_B |
+					    DRM_MODE_TV_NORM_PAL_M,
+					    0, NULL);
 	if (ret)
 		return ret;