diff mbox series

[2/3] drm/vc4: Add monochrome mode to the VEC.

Message ID 20240216184857.245372-3-dave.stevenson@raspberrypi.com (mailing list archive)
State New, archived
Headers show
Series vc4 VEC (analogue video) updates - margins and monochrome | expand

Commit Message

Dave Stevenson Feb. 16, 2024, 6:48 p.m. UTC 
The VEC supports not producing colour bursts for monochrome output.
It also has an option for disabling the chroma input to remove
chroma from the signal.

Now that there is a DRM_MODE_TV_MODE_MONOCHROME defined, plumb
this in.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/vc4/vc4_vec.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Comments

Maxime Ripard June 18, 2024, 9:28 a.m. UTC | #1 
Hi,

On Fri, Feb 16, 2024 at 06:48:56PM GMT, Dave Stevenson wrote:
> The VEC supports not producing colour bursts for monochrome output.
> It also has an option for disabling the chroma input to remove
> chroma from the signal.
> 
> Now that there is a DRM_MODE_TV_MODE_MONOCHROME defined, plumb
> this in.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/gpu/drm/vc4/vc4_vec.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> index 268f18b10ee0..f9e134dd1e3b 100644
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -234,6 +234,7 @@ enum vc4_vec_tv_mode_id {
>  	VC4_VEC_TV_MODE_PAL_60,
>  	VC4_VEC_TV_MODE_PAL_N,
>  	VC4_VEC_TV_MODE_SECAM,
> +	VC4_VEC_TV_MODE_MONOCHROME,
>  };
>  
>  struct vc4_vec_tv_mode {
> @@ -324,6 +325,22 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>  		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
>  		.custom_freq = 0x29c71c72,
>  	},
> +	{
> +		/* 50Hz mono */
> +		.mode = DRM_MODE_TV_MODE_MONOCHROME,
> +		.expected_htotal = 864,
> +		.config0 = VEC_CONFIG0_PAL_BDGHI_STD | VEC_CONFIG0_BURDIS |
> +			   VEC_CONFIG0_CHRDIS,
> +		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
> +	},
> +	{
> +		/* 60Hz mono */
> +		.mode = DRM_MODE_TV_MODE_MONOCHROME,
> +		.expected_htotal = 858,
> +		.config0 = VEC_CONFIG0_PAL_M_STD | VEC_CONFIG0_BURDIS |
> +			   VEC_CONFIG0_CHRDIS,
> +		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
> +	},
>  };
>  
>  static inline const struct vc4_vec_tv_mode *
> @@ -351,6 +368,7 @@ static const struct drm_prop_enum_list legacy_tv_mode_names[] = {
>  	{ VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
>  	{ VC4_VEC_TV_MODE_PAL_N, "PAL-N", },
>  	{ VC4_VEC_TV_MODE_SECAM, "SECAM", },
> +	{ VC4_VEC_TV_MODE_MONOCHROME, "Mono", },
>  };
>  
>  static enum drm_connector_status
> @@ -406,6 +424,10 @@ vc4_vec_connector_set_property(struct drm_connector *connector,
>  		state->tv.mode = DRM_MODE_TV_MODE_SECAM;
>  		break;
>  
> +	case VC4_VEC_TV_MODE_MONOCHROME:
> +		state->tv.mode = DRM_MODE_TV_MODE_MONOCHROME;
> +		break;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -453,6 +475,9 @@ vc4_vec_connector_get_property(struct drm_connector *connector,
>  		*val = VC4_VEC_TV_MODE_SECAM;
>  		break;
>  
> +	case DRM_MODE_TV_MODE_MONOCHROME:
> +		return VC4_VEC_TV_MODE_MONOCHROME;
> +
>  	default:
>  		return -EINVAL;
>  	}

We don't need to expose the new value here, that property is only for
the legacy, driver-specific property. So you should only need the
vc4_vec_tv_modes changes

Maxime
Dave Stevenson June 18, 2024, 6:15 p.m. UTC | #2 
Hi Maxime

On Tue, 18 Jun 2024 at 10:28, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Fri, Feb 16, 2024 at 06:48:56PM GMT, Dave Stevenson wrote:
> > The VEC supports not producing colour bursts for monochrome output.
> > It also has an option for disabling the chroma input to remove
> > chroma from the signal.
> >
> > Now that there is a DRM_MODE_TV_MODE_MONOCHROME defined, plumb
> > this in.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_vec.c | 28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> > index 268f18b10ee0..f9e134dd1e3b 100644
> > --- a/drivers/gpu/drm/vc4/vc4_vec.c
> > +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> > @@ -234,6 +234,7 @@ enum vc4_vec_tv_mode_id {
> >       VC4_VEC_TV_MODE_PAL_60,
> >       VC4_VEC_TV_MODE_PAL_N,
> >       VC4_VEC_TV_MODE_SECAM,
> > +     VC4_VEC_TV_MODE_MONOCHROME,
> >  };
> >
> >  struct vc4_vec_tv_mode {
> > @@ -324,6 +325,22 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
> >               .config1 = VEC_CONFIG1_C_CVBS_CVBS,
> >               .custom_freq = 0x29c71c72,
> >       },
> > +     {
> > +             /* 50Hz mono */
> > +             .mode = DRM_MODE_TV_MODE_MONOCHROME,
> > +             .expected_htotal = 864,
> > +             .config0 = VEC_CONFIG0_PAL_BDGHI_STD | VEC_CONFIG0_BURDIS |
> > +                        VEC_CONFIG0_CHRDIS,
> > +             .config1 = VEC_CONFIG1_C_CVBS_CVBS,
> > +     },
> > +     {
> > +             /* 60Hz mono */
> > +             .mode = DRM_MODE_TV_MODE_MONOCHROME,
> > +             .expected_htotal = 858,
> > +             .config0 = VEC_CONFIG0_PAL_M_STD | VEC_CONFIG0_BURDIS |
> > +                        VEC_CONFIG0_CHRDIS,
> > +             .config1 = VEC_CONFIG1_C_CVBS_CVBS,
> > +     },
> >  };
> >
> >  static inline const struct vc4_vec_tv_mode *
> > @@ -351,6 +368,7 @@ static const struct drm_prop_enum_list legacy_tv_mode_names[] = {
> >       { VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
> >       { VC4_VEC_TV_MODE_PAL_N, "PAL-N", },
> >       { VC4_VEC_TV_MODE_SECAM, "SECAM", },
> > +     { VC4_VEC_TV_MODE_MONOCHROME, "Mono", },
> >  };
> >
> >  static enum drm_connector_status
> > @@ -406,6 +424,10 @@ vc4_vec_connector_set_property(struct drm_connector *connector,
> >               state->tv.mode = DRM_MODE_TV_MODE_SECAM;
> >               break;
> >
> > +     case VC4_VEC_TV_MODE_MONOCHROME:
> > +             state->tv.mode = DRM_MODE_TV_MODE_MONOCHROME;
> > +             break;
> > +
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -453,6 +475,9 @@ vc4_vec_connector_get_property(struct drm_connector *connector,
> >               *val = VC4_VEC_TV_MODE_SECAM;
> >               break;
> >
> > +     case DRM_MODE_TV_MODE_MONOCHROME:
> > +             return VC4_VEC_TV_MODE_MONOCHROME;

I have got an error here - it should be
 *val = VC4_VEC_TV_MODE_MONOCHROME;
  break;

> > +
> >       default:
> >               return -EINVAL;
> >       }
>
> We don't need to expose the new value here, that property is only for
> the legacy, driver-specific property. So you should only need the
> vc4_vec_tv_modes changes

As both properties share the same underlying value, that means that if
the new property selects Monochrome, the legacy one will return
-EINVAL as it is an unknown value.

modetest and kmsprint -p both bomb out in this situation as by the
looks of it we fail to get a pointer to the connector returned. That
means you can't switch it back again.

Am I missing something?

  Dave
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index 268f18b10ee0..f9e134dd1e3b 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -234,6 +234,7 @@  enum vc4_vec_tv_mode_id {
 	VC4_VEC_TV_MODE_PAL_60,
 	VC4_VEC_TV_MODE_PAL_N,
 	VC4_VEC_TV_MODE_SECAM,
+	VC4_VEC_TV_MODE_MONOCHROME,
 };
 
 struct vc4_vec_tv_mode {
@@ -324,6 +325,22 @@  static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 		.custom_freq = 0x29c71c72,
 	},
+	{
+		/* 50Hz mono */
+		.mode = DRM_MODE_TV_MODE_MONOCHROME,
+		.expected_htotal = 864,
+		.config0 = VEC_CONFIG0_PAL_BDGHI_STD | VEC_CONFIG0_BURDIS |
+			   VEC_CONFIG0_CHRDIS,
+		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
+	},
+	{
+		/* 60Hz mono */
+		.mode = DRM_MODE_TV_MODE_MONOCHROME,
+		.expected_htotal = 858,
+		.config0 = VEC_CONFIG0_PAL_M_STD | VEC_CONFIG0_BURDIS |
+			   VEC_CONFIG0_CHRDIS,
+		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
+	},
 };
 
 static inline const struct vc4_vec_tv_mode *
@@ -351,6 +368,7 @@  static const struct drm_prop_enum_list legacy_tv_mode_names[] = {
 	{ VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
 	{ VC4_VEC_TV_MODE_PAL_N, "PAL-N", },
 	{ VC4_VEC_TV_MODE_SECAM, "SECAM", },
+	{ VC4_VEC_TV_MODE_MONOCHROME, "Mono", },
 };
 
 static enum drm_connector_status
@@ -406,6 +424,10 @@  vc4_vec_connector_set_property(struct drm_connector *connector,
 		state->tv.mode = DRM_MODE_TV_MODE_SECAM;
 		break;
 
+	case VC4_VEC_TV_MODE_MONOCHROME:
+		state->tv.mode = DRM_MODE_TV_MODE_MONOCHROME;
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -453,6 +475,9 @@  vc4_vec_connector_get_property(struct drm_connector *connector,
 		*val = VC4_VEC_TV_MODE_SECAM;
 		break;
 
+	case DRM_MODE_TV_MODE_MONOCHROME:
+		return VC4_VEC_TV_MODE_MONOCHROME;
+
 	default:
 		return -EINVAL;
 	}
@@ -754,7 +779,8 @@  static int vc4_vec_bind(struct device *dev, struct device *master, void *data)
 					    BIT(DRM_MODE_TV_MODE_PAL) |
 					    BIT(DRM_MODE_TV_MODE_PAL_M) |
 					    BIT(DRM_MODE_TV_MODE_PAL_N) |
-					    BIT(DRM_MODE_TV_MODE_SECAM));
+					    BIT(DRM_MODE_TV_MODE_SECAM) |
+					    BIT(DRM_MODE_TV_MODE_MONOCHROME));
 	if (ret)
 		return ret;