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