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

[Dave Stevenson]

Hi Maxime

On Tue, 18 Jun 2024 at 10:28, Maxime Ripard <mripard at 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 at 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