Hi Maxime
On Fri, 9 Apr 2021 at 13:36, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> Hi Dave,
>
> On Thu, Apr 01, 2021 at 12:30:45PM +0100, Dave Stevenson wrote:
> > > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/vc4/vc4_hdmi.c | 56 +++++++++++++++++++++++++++++
> > > drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 3 ++
> > > 2 files changed, 59 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index 0924a1b9e186..530c83097b1a 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -35,6 +35,7 @@
> > > #include <drm/drm_edid.h>
> > > #include <drm/drm_probe_helper.h>
> > > #include <drm/drm_simple_kms_helper.h>
> > > +#include <drm/drm_scdc_helper.h>
> > > #include <linux/clk.h>
> > > #include <linux/component.h>
> > > #include <linux/i2c.h>
> > > @@ -76,6 +77,8 @@
> > > #define VC5_HDMI_VERTB_VSPO_SHIFT 16
> > > #define VC5_HDMI_VERTB_VSPO_MASK VC4_MASK(29, 16)
> > >
> > > +#define VC5_HDMI_SCRAMBLER_CTL_ENABLE BIT(0)
> > > +
> > > #define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_SHIFT 8
> > > #define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_MASK VC4_MASK(10, 8)
> > >
> > > @@ -457,6 +460,56 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
> > > vc4_hdmi_set_audio_infoframe(encoder);
> > > }
> > >
> > > +static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder,
> > > + struct drm_display_mode *mode)
> > > +{
> > > + struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
> > > + struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
> > > + struct drm_display_info *display = &vc4_hdmi->connector.display_info;
> > > +
> > > + if (!vc4_encoder->hdmi_monitor)
> > > + return false;
> > > +
> > > + if (!display->hdmi.scdc.supported ||
> > > + !display->hdmi.scdc.scrambling.supported)
> > > + return false;
> > > +
> >
> > I think I made this comment last time, but possibly not very clearly.
>
> I must have missed it then, sorry :/
>
> > Up to this point in the function is whether the display/hdmi interface
> > *supports* scrambling. The bit after is whether it is *required* to be
> > enabled by the mode.
>
> Thomas made a suggestion to move the mode clock check to a helper, so
> we'll end up with essentially a helper about whether we support
> scrambling or not and if the mode requires it.
>
> > At the moment, if the display/interface supports scrambling but the
> > mode doesn't need it, then the scrambling setup is never touched. That
> > makes a few assumptions about the default settings.
> > Boot with the firmware selecting 4k60 (ie scrambling on), but
> > video=HDMI-1:1920x1080 in the kernel command line and you'll have a
> > mess with scrambling enabled but not signalled.
> >
> > I'd be happier if the display/interface says scrambling is supported
> > then we always call drm_scdc_set_high_tmds_clock_ratio,
> > drm_scdc_set_scrambling and set the VC5_HDMI_SCRAMBLER_CTL_ENABLE
> > register bit appropriately for the mode. Feel free to disagree with me
> > though.
>
> I think part of it is due to our custom helpers never being called
> currently during the boot process. Once that is fixed, the disable
> helpers will be called and will disable the scrambling so we should be
> good there.
>
> This creates another issue though. That function takes the mode as the
> argument and at boot time the mode pointer will be null. I think we can
> work around it by assuming that we need to disable it at boot all the
> time (and thus ignore the test if our pointer is null).
>
> Would that work for you?
Yes, that all sounds reasonable to me.
Dave
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
