[Intel-gfx] [PATCH 11/18] drm/i915: Return the mask of enabled infoframes from ->inforame_enabled()
Daniel Vetter
daniel at ffwll.ch
Mon Oct 1 06:55:51 UTC 2018
On Mon, Sep 24, 2018 at 07:36:08PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 05:51:16PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 20, 2018 at 09:51:38PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > We want to start tracking which infoframes are enabled, so let's replace
> > > the boolean flag with a bitmask.
> > >
> > > We'll abstract the bitmask so that it's not platform dependent. That
> > > will allow us to examine the bitmask later in platform independent code.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_ddi.c | 2 +-
> > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > drivers/gpu/drm/i915/intel_hdmi.c | 87 ++++++++++++++++++++++++++++-----------
> > > 3 files changed, 68 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 086e3f940586..098a0e4edf2a 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -3390,7 +3390,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> > > pipe_config->has_hdmi_sink = true;
> > > intel_dig_port = enc_to_dig_port(&encoder->base);
> > >
> > > - if (intel_dig_port->infoframe_enabled(encoder, pipe_config))
> > > + if (intel_hdmi_infoframes_enabled(encoder, pipe_config))
> > > pipe_config->has_infoframe = true;
> > >
> > > if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index e0f3a79fc75e..6815c69aac2f 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1181,7 +1181,7 @@ struct intel_digital_port {
> > > bool enable,
> > > const struct intel_crtc_state *crtc_state,
> > > const struct drm_connector_state *conn_state);
> > > - bool (*infoframe_enabled)(struct intel_encoder *encoder,
> > > + u32 (*infoframes_enabled)(struct intel_encoder *encoder,
> > > const struct intel_crtc_state *pipe_config);
> > > };
> > >
> > > @@ -1856,6 +1856,8 @@ bool intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
> > > bool scrambling);
> > > void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> > > void intel_infoframe_init(struct intel_digital_port *intel_dig_port);
> > > +u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state);
> > >
> > >
> > > /* intel_lvds.c */
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index c3c2a638d062..a8fcddb199ae 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -100,10 +100,14 @@ static u32 g4x_infoframe_index(unsigned int type)
> > > static u32 g4x_infoframe_enable(unsigned int type)
> > > {
> > > switch (type) {
> > > + case HDMI_PACKET_TYPE_NULL:
> > > + return VIDEO_DIP_ENABLE; /* slight lie */
> >
> > Not exactly sure why we're tracking this one here, but not for hsw.
>
> HSW+ doesn't have a DIP enable bit like this. It only has the
> bits to enable specific infoframes.
>
> > Shouldn't we include that one if the DDI port is in hdmi mode? Would be
> > more consistent I think.
>
> Yes that would seem like the more correct thing. I think the reason
> I did this here was so that I could map the DIP_ENABLE bit to
> something unique. Would allow us to differentiate between the
> "DIP enabled with no infoframes enabled" vs. "DIP enabled with
> some infoframes enabled" cases. But seeing as we always enable some
> infoframes I guess this doesn't really provide us with anything
> particularly useful.
>
> That said, I'm actually not sure whether the hw will send the null
> packets if we don't enable the DIP. Would require a HDMI analyzer
> to confirm.
>
> Hmm. Actually gen4 bspec tells me:
> "If DIP is enabled but DIP types are all disabled, no DIP is sent.
> However, a single Null DIP will be sent at the same point in the
> stream that DIP packets would have been sent. This is done to
> keep the port in HDMI mode, otherwise it would revert to DVI mode.
> The "Null packets enabled during vsync" mode (bit #9 of port
> control register) overrides this behavior."
>
> So I guess mapping the null packet to the DIP enable bit is more or
> less correct. Although the spec doesn't quite say whether the null
> packet is also sent when some DIP types are also enabled, or if it
> is only send when no DIP types are enabled.
>
> So to match the hw I guess the readout should really do something
> like:
>
> if (hdmi & HDMI_MODE || dip_ctl & DIP_ENABLE)
> infoframes |= TYPE_NULL;
>
> but that would again mean that we can't tell the two cases
> apart.
>From a functionality pov, do we actually care about the null DIP? HDMI
mode y/n for sure, but we track that already. Feels like just never
reading out the null packet state and never setting it in the state
structure is the cheap way out here, and also the least confusing way out
here. Trying to second guess what the hw does for something that doesn't
seem to matter feels a bit silly.
-Daniel
>
> >
> > Aside from that lgtm, has my
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> >
> > once we figured the TYPE_NULL thing out.
> > -Daniel
> >
> > > case HDMI_PACKET_TYPE_GENERAL_CONTROL:
> > > return VIDEO_DIP_ENABLE_GCP;
> > > case HDMI_PACKET_TYPE_GAMUT_METADATA:
> > > return VIDEO_DIP_ENABLE_GAMUT;
> > > + case DP_SDP_VSC:
> > > + return 0;
> > > case HDMI_INFOFRAME_TYPE_AVI:
> > > return VIDEO_DIP_ENABLE_AVI;
> > > case HDMI_INFOFRAME_TYPE_SPD:
> > > @@ -119,6 +123,8 @@ static u32 g4x_infoframe_enable(unsigned int type)
> > > static u32 hsw_infoframe_enable(unsigned int type)
> > > {
> > > switch (type) {
> > > + case HDMI_PACKET_TYPE_NULL:
> > > + return 0;
> > > case HDMI_PACKET_TYPE_GENERAL_CONTROL:
> > > return VIDEO_DIP_ENABLE_GCP_HSW;
> > > case HDMI_PACKET_TYPE_GAMUT_METADATA:
> > > @@ -197,19 +203,19 @@ static void g4x_write_infoframe(struct intel_encoder *encoder,
> > > POSTING_READ(VIDEO_DIP_CTL);
> > > }
> > >
> > > -static bool g4x_infoframe_enabled(struct intel_encoder *encoder,
> > > +static u32 g4x_infoframes_enabled(struct intel_encoder *encoder,
> > > const struct intel_crtc_state *pipe_config)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > u32 val = I915_READ(VIDEO_DIP_CTL);
> > >
> > > if ((val & VIDEO_DIP_ENABLE) == 0)
> > > - return false;
> > > + return 0;
> > >
> > > if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(encoder->port))
> > > - return false;
> > > + return 0;
> > >
> > > - return val & (VIDEO_DIP_ENABLE_AVI |
> > > + return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_SPD);
> > > }
> > >
> > > @@ -252,7 +258,7 @@ static void ibx_write_infoframe(struct intel_encoder *encoder,
> > > POSTING_READ(reg);
> > > }
> > >
> > > -static bool ibx_infoframe_enabled(struct intel_encoder *encoder,
> > > +static u32 ibx_infoframes_enabled(struct intel_encoder *encoder,
> > > const struct intel_crtc_state *pipe_config)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > @@ -261,12 +267,12 @@ static bool ibx_infoframe_enabled(struct intel_encoder *encoder,
> > > u32 val = I915_READ(reg);
> > >
> > > if ((val & VIDEO_DIP_ENABLE) == 0)
> > > - return false;
> > > + return 0;
> > >
> > > if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(encoder->port))
> > > - return false;
> > > + return 0;
> > >
> > > - return val & (VIDEO_DIP_ENABLE_AVI |
> > > + return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > }
> > > @@ -313,7 +319,7 @@ static void cpt_write_infoframe(struct intel_encoder *encoder,
> > > POSTING_READ(reg);
> > > }
> > >
> > > -static bool cpt_infoframe_enabled(struct intel_encoder *encoder,
> > > +static u32 cpt_infoframes_enabled(struct intel_encoder *encoder,
> > > const struct intel_crtc_state *pipe_config)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > @@ -321,9 +327,9 @@ static bool cpt_infoframe_enabled(struct intel_encoder *encoder,
> > > u32 val = I915_READ(TVIDEO_DIP_CTL(pipe));
> > >
> > > if ((val & VIDEO_DIP_ENABLE) == 0)
> > > - return false;
> > > + return 0;
> > >
> > > - return val & (VIDEO_DIP_ENABLE_AVI |
> > > + return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > }
> > > @@ -367,7 +373,7 @@ static void vlv_write_infoframe(struct intel_encoder *encoder,
> > > POSTING_READ(reg);
> > > }
> > >
> > > -static bool vlv_infoframe_enabled(struct intel_encoder *encoder,
> > > +static u32 vlv_infoframes_enabled(struct intel_encoder *encoder,
> > > const struct intel_crtc_state *pipe_config)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > @@ -375,12 +381,12 @@ static bool vlv_infoframe_enabled(struct intel_encoder *encoder,
> > > u32 val = I915_READ(VLV_TVIDEO_DIP_CTL(pipe));
> > >
> > > if ((val & VIDEO_DIP_ENABLE) == 0)
> > > - return false;
> > > + return 0;
> > >
> > > if ((val & VIDEO_DIP_PORT_MASK) != VIDEO_DIP_PORT(encoder->port))
> > > - return false;
> > > + return 0;
> > >
> > > - return val & (VIDEO_DIP_ENABLE_AVI |
> > > + return val & (VIDEO_DIP_ENABLE | VIDEO_DIP_ENABLE_AVI |
> > > VIDEO_DIP_ENABLE_VENDOR | VIDEO_DIP_ENABLE_GAMUT |
> > > VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_ENABLE_GCP);
> > > }
> > > @@ -419,7 +425,7 @@ static void hsw_write_infoframe(struct intel_encoder *encoder,
> > > POSTING_READ(ctl_reg);
> > > }
> > >
> > > -static bool hsw_infoframe_enabled(struct intel_encoder *encoder,
> > > +static u32 hsw_infoframes_enabled(struct intel_encoder *encoder,
> > > const struct intel_crtc_state *pipe_config)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > @@ -430,6 +436,42 @@ static bool hsw_infoframe_enabled(struct intel_encoder *encoder,
> > > VIDEO_DIP_ENABLE_GMP_HSW | VIDEO_DIP_ENABLE_SPD_HSW);
> > > }
> > >
> > > +static const u8 infoframe_type_to_idx[] = {
> > > + HDMI_PACKET_TYPE_NULL,
> > > + HDMI_PACKET_TYPE_GENERAL_CONTROL,
> > > + HDMI_PACKET_TYPE_GAMUT_METADATA,
> > > + DP_SDP_VSC,
> > > + HDMI_INFOFRAME_TYPE_AVI,
> > > + HDMI_INFOFRAME_TYPE_SPD,
> > > + HDMI_INFOFRAME_TYPE_VENDOR,
> > > +};
> > > +
> > > +u32 intel_hdmi_infoframes_enabled(struct intel_encoder *encoder,
> > > + const struct intel_crtc_state *crtc_state)
> > > +{
> > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > > + u32 val, ret = 0;
> > > + int i;
> > > +
> > > + val = dig_port->infoframes_enabled(encoder, crtc_state);
> > > +
> > > + /* map from hardware bits to dip idx */
> > > + for (i = 0; i < ARRAY_SIZE(infoframe_type_to_idx); i++) {
> > > + unsigned int type = infoframe_type_to_idx[i];
> > > +
> > > + if (HAS_DDI(dev_priv)) {
> > > + if (val & hsw_infoframe_enable(type))
> > > + ret |= BIT(i);
> > > + } else {
> > > + if (val & g4x_infoframe_enable(type))
> > > + ret |= BIT(i);
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /*
> > > * The data we write to the DIP data buffer registers is 1 byte bigger than the
> > > * HDMI infoframe size because of an ECC/reserved byte at position 3 (starting
> > > @@ -1200,7 +1242,6 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
> > > struct intel_crtc_state *pipe_config)
> > > {
> > > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> > > - struct intel_digital_port *intel_dig_port = hdmi_to_dig_port(intel_hdmi);
> > > struct drm_device *dev = encoder->base.dev;
> > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > u32 tmp, flags = 0;
> > > @@ -1223,7 +1264,7 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
> > > if (tmp & HDMI_MODE_SELECT_HDMI)
> > > pipe_config->has_hdmi_sink = true;
> > >
> > > - if (intel_dig_port->infoframe_enabled(encoder, pipe_config))
> > > + if (intel_hdmi_infoframes_enabled(encoder, pipe_config))
> > > pipe_config->has_infoframe = true;
> > >
> > > if (tmp & SDVO_AUDIO_ENABLE)
> > > @@ -2325,23 +2366,23 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > intel_dig_port->write_infoframe = vlv_write_infoframe;
> > > intel_dig_port->set_infoframes = vlv_set_infoframes;
> > > - intel_dig_port->infoframe_enabled = vlv_infoframe_enabled;
> > > + intel_dig_port->infoframes_enabled = vlv_infoframes_enabled;
> > > } else if (IS_G4X(dev_priv)) {
> > > intel_dig_port->write_infoframe = g4x_write_infoframe;
> > > intel_dig_port->set_infoframes = g4x_set_infoframes;
> > > - intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
> > > + intel_dig_port->infoframes_enabled = g4x_infoframes_enabled;
> > > } else if (HAS_DDI(dev_priv)) {
> > > intel_dig_port->write_infoframe = hsw_write_infoframe;
> > > intel_dig_port->set_infoframes = hsw_set_infoframes;
> > > - intel_dig_port->infoframe_enabled = hsw_infoframe_enabled;
> > > + intel_dig_port->infoframes_enabled = hsw_infoframes_enabled;
> > > } else if (HAS_PCH_IBX(dev_priv)) {
> > > intel_dig_port->write_infoframe = ibx_write_infoframe;
> > > intel_dig_port->set_infoframes = ibx_set_infoframes;
> > > - intel_dig_port->infoframe_enabled = ibx_infoframe_enabled;
> > > + intel_dig_port->infoframes_enabled = ibx_infoframes_enabled;
> > > } else {
> > > intel_dig_port->write_infoframe = cpt_write_infoframe;
> > > intel_dig_port->set_infoframes = cpt_set_infoframes;
> > > - intel_dig_port->infoframe_enabled = cpt_infoframe_enabled;
> > > + intel_dig_port->infoframes_enabled = cpt_infoframes_enabled;
> > > }
> > > }
> > >
> > > --
> > > 2.16.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list