On Fri, 2017-03-03 at 17:33 +0530, Sharma, Shashank wrote:
> Thanks for the review Ander. My comments inline.
>
> Shashank
>
> On 3/3/2017 3:22 PM, Ander Conselvan De Oliveira wrote:
> > On Fri, 2017-03-03 at 11:59 +0530, Shashank Sharma wrote:
> > > Geminilake platform sports a native HDMI 2.0 controller, and is
> > > capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
> > > mendates scrambling for these higher clocks, for reduced RF footprint.
> > >
> > > This patch checks if the monitor supports scrambling, and if required,
> > > enables it during the modeset.
> > >
> > > V2: Addressed review comments from Ville:
> > > - Do not track scrambling status in DRM layer, track somewhere in
> > > driver like in intel_crtc_state.
> > > - Don't talk to monitor at such a low layer, set monitor scrambling
> > > in intel_enable_ddi() before enabling the port.
> > >
> > > V3: Addressed review comments from Jani
> > > - In comments, function names, use "sink" instead of "monitor",
> > > so that the implementation could be close to the language of
> > > HDMI spec.
> > >
> > > V4: Addressed review comment from Maarten
> > > - scrambling -> hdmi_scrambling
> > > high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> > >
> > > V5: Addressed review comments from Ville and Ander
> > > - Do not modifiy the crtc_state after compute_config. Move all
> > > scrambling and tmds_clock_ratio calcutations to compute_config.
> > > - While setting scrambling for source/sink, do not check the
> > > conditions again, just go by the crtc_state flags. This will
> > > simplyfy the condition checks.
> > >
> > > V6: Addressed review comments from Ville
> > > - Do not add IS_GLK check in disable/enable function, instead add it
> > > in compute_config, while setting state flags.
> > > - Remove unnecessary paranthesis.
> > > - Simplyfy handle_sink_scrambling function as suggested.
> > > - Add readout code for scrambling status in get_ddi_config and add a
> > > check for the same in pipe_config_compare.
> > >
> > > Signed-off-by: Shashank Sharma <shashank.sha...@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_reg.h | 7 ++++
> > > drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++++
> > > drivers/gpu/drm/i915/intel_display.c | 5 +++
> > > drivers/gpu/drm/i915/intel_drv.h | 14 +++++++
> > > drivers/gpu/drm/i915/intel_hdmi.c | 74
> > > ++++++++++++++++++++++++++++++++++++
> > > 5 files changed, 133 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 4906ce4d..f7891ac 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7824,7 +7824,14 @@ enum {
> > > #define TRANS_DDI_EDP_INPUT_B_ONOFF (5<<12)
> > > #define TRANS_DDI_EDP_INPUT_C_ONOFF (6<<12)
> > > #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1<<8)
> > > +#define TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> > > +#define TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
> > > #define TRANS_DDI_BFI_ENABLE (1<<4)
> > > +#define TRANS_DDI_HIGH_TMDS_CHAR_RATE (1<<4)
> > > +#define TRANS_DDI_HDMI_SCRAMBLING (1<<0)
> > > +#define TRANS_DDI_HDMI_SCRAMBLING_MASK
> > > (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
> > > + | TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
> > > + | TRANS_DDI_HDMI_SCRAMBLING)
> >
> > Last line is misaligned.
>
> Ok.
> > >
> > > /* DisplayPort Transport Control */
> > > #define _DP_TP_CTL_A 0x64040
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index a7c08d7..d0c6927 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1311,6 +1311,11 @@ void intel_ddi_enable_transcoder_func(struct
> > > drm_crtc *crtc)
> > > temp |= TRANS_DDI_MODE_SELECT_HDMI;
> > > else
> > > temp |= TRANS_DDI_MODE_SELECT_DVI;
> > > +
> > > + if (IS_GEMINILAKE(dev_priv))
> > > + temp = intel_hdmi_handle_source_scrambling(
> > > + intel_encoder,
> > > + intel_crtc->config, temp);
> >
> > This whole hunk could just be:
> >
> > + if (config->hdmi_scrambling)
> > + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
> > +
> > + if (config->hdmi_high_tmds_clock_ratio)
> > + hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> >
> > No need to check for GLK here, since the compute config code already did.
>
> As I was emphasizing in the previous review comments, having two
> separate functions, one to set source_scrambling and
> other to set sink_scrambling makes it easy to follow up, due to design
> symmetry, and in that case you need not to refer
> to bspec to find out which all registers be dealing with scrambling ,
> just check for scrambling control functions. So I was insisting
> on keeping them.
But that design creates an asymmetry when it comes to checking all bits that are
enabled in TRANS_DDI_FUNC_CTL() that involves function-hopping. Maybe it is just
a matter of different taste. But for the more practical problem of finding what
bits are involved in enabling scrambling, I think
git grep hdmi_scrambling
will do a good enough job.
>
> But anyways, both you and Ville are insisting on keeping it inline, so
> lets keep it that way.
> > > } else if (type == INTEL_OUTPUT_ANALOG) {
> > > temp |= TRANS_DDI_MODE_SELECT_FDI;
> > > temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> > > @@ -1885,6 +1890,21 @@ static void intel_enable_ddi(struct intel_encoder
> > > *intel_encoder,
> > > struct intel_digital_port *intel_dig_port =
> > > enc_to_dig_port(encoder);
> > >
> > > + if (IS_GEMINILAKE(dev_priv)) {
> > > + struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > + /*
> > > + * GLK sports a native HDMI 2.0 controller. If required
> > > + * clock rate is > 340 Mhz && scrambling is supported
> > > + * by sink, enable scrambling before enabling the
> > > + * HDMI 2.0 port. The sink can choose to disable the
> > > + * scrambling if it doesn't detect a scrambled within
> > > + * 100 ms.
> > > + */
> > > + intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > + conn_state->connector,
> > > + crtc->config, true);
> >
> > This function receives a pipe_config pointer as argument. Use that instead
> > of
> > crtc->config.
>
> Ok.
> > > + }
> > > +
> > > /* In HDMI/DVI mode, the port width, and swing/emphasis
> > > values
> > > * are ignored so nothing special needs to be done
> > > besides
> > > * enabling the port.
> > > @@ -1917,6 +1937,14 @@ static void intel_disable_ddi(struct intel_encoder
> > > *intel_encoder,
> > > if (old_crtc_state->has_audio)
> > > intel_audio_codec_disable(intel_encoder);
> > >
> > > + if (type == INTEL_OUTPUT_HDMI) {
> > > + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > > +
> > > + intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > + old_conn_state->connector,
> > > + intel_crtc->config, false);
> > > + }
> > > +
> > > if (type == INTEL_OUTPUT_EDP) {
> > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >
> > > @@ -2044,6 +2072,11 @@ void intel_ddi_get_config(struct intel_encoder
> > > *encoder,
> > >
> > > if (intel_hdmi->infoframe_enabled(&encoder->base,
> > > pipe_config))
> > > pipe_config->has_infoframe = true;
> > > +
> > > + if (temp & TRANS_DDI_HDMI_SCRAMBLING_MASK)
I missed this the first time, but we say hdmi_scrambling is enabled if any of
those bits in the mask is set. Is that really what we want? Or should this check
specifically for the TRANS_DDI_HDMI_SCRAMBLING bit or all the bits set?
> > > + pipe_config->hdmi_scrambling = true;
> > > + if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
> > > + pipe_config->hdmi_high_tmds_clock_ratio = true;
> >
> > Hmm, we might need a IS_GLK here, since these bits only exist in that
> > platform.
> > I believe they should read zero though, since they are listed are reserved.
> > Dunno.
>
> Hmm, I dont think there would be a way to set the reserved bits though,
> so we can keep it. In this way the
> only place we are checking and setting the platform is compute_config.
Yeah, with the change to check the state for non-GLK too, we get a WARN if this
fails, so seems ok.
> > > /* fall through */
> > > case TRANS_DDI_MODE_SELECT_DVI:
> > > pipe_config->lane_count = 4;
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 99e8d9c..c017ed3 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11662,6 +11662,11 @@ static void __printf(3, 4)
> > > if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
> > > IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > PIPE_CONF_CHECK_I(limited_color_range);
> > > + if (IS_GEMINILAKE(dev_priv)) {
> >
> > No need to check for GLK here. The configuration should never be set for
> > other
> > platforms. If it is set, that's a bug and we want verify_crtc_state() to
> > tells
> > us about it.
>
> Agree, will remove this.
> >
> > > + PIPE_CONF_CHECK_I(hdmi_scrambling);
> > > + PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
> > > + }
> > > +
> > > PIPE_CONF_CHECK_I(has_infoframe);
> > >
> > > PIPE_CONF_CHECK_I(has_audio);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index e6fbbee..baefbaa 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -695,6 +695,12 @@ struct intel_crtc_state {
> > >
> > > /* Gamma mode programmed on the pipe */
> > > uint32_t gamma_mode;
> > > +
> > > + /* HDMI scrambling status (sink) */
> > > + bool hdmi_scrambling;
> > > +
> > > + /* HDMI High TMDS char rate ratio (sink) */
> > > + bool hdmi_high_tmds_clock_ratio;
> > > };
> > >
> > > struct vlv_wm_state {
> > > @@ -1626,6 +1632,14 @@ void intel_hdmi_init_connector(struct
> > > intel_digital_port *intel_dig_port,
> > > bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > > struct intel_crtc_state *pipe_config,
> > > struct drm_connector_state *conn_state);
> > > +uint32_t
> > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > + struct intel_crtc_state *config,
> > > + uint32_t hdmi_config);
> > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder
> > > *intel_encoder,
> > > + struct drm_connector *connector,
> > > + struct intel_crtc_state *config,
> > > + bool enable);
> > > void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool
> > > enable);
> > >
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index c2184f7..0a1ae03 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -34,6 +34,7 @@
> > > #include <drm/drm_atomic_helper.h>
> > > #include <drm/drm_crtc.h>
> > > #include <drm/drm_edid.h>
> > > +#include <drm/drm_scdc_helper.h>
> > > #include "intel_drv.h"
> > > #include <drm/i915_drm.h>
> > > #include <drm/intel_lpe_audio.h>
> > > @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder
> > > *encoder,
> > > struct intel_hdmi *intel_hdmi =
> > > enc_to_intel_hdmi(&encoder->base);
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > struct drm_display_mode *adjusted_mode =
> > > &pipe_config->base.adjusted_mode;
> > > + struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
> > > int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> > > int clock_12bpc = clock_8bpc * 3 / 2;
> > > int desired_bpp;
> > > @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct
> > > intel_encoder *encoder,
> > >
> > > pipe_config->lane_count = 4;
> > >
> > > + if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
> > > + if (scdc->scrambling.low_rates)
> > > + pipe_config->hdmi_scrambling = true;
> > > +
> > > + if (pipe_config->port_clock > 340000) {
> > > + pipe_config->hdmi_scrambling = true;
> > > + pipe_config->hdmi_high_tmds_clock_ratio = true;
> > > + }
> > > + }
> > > +
> > > return true;
> > > }
> > >
> > > @@ -1794,6 +1806,68 @@ static void intel_hdmi_destroy(struct
> > > drm_connector *connector)
> > > intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > > }
> > >
> > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder
> > > *intel_encoder,
> > > + struct drm_connector *connector,
> > > + struct intel_crtc_state *config,
> > > + bool enable)
> > > +{
> > > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> > > + struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > > + struct drm_scrambling *scrambling =
> > > + &connector->display_info.hdmi.scdc.scrambling;
> > > + struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> > > + intel_hdmi->ddc_bus);
> > > + bool ret;
> > > +
> > > + if (!scrambling->supported)
> > > + return;
> > > +
> > > + DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> > > + intel_encoder->base.name, connector->name);
> > > +
> > > + if (config->hdmi_high_tmds_clock_ratio) {
> > > + /* Set TMDS bit clock ratio to 1/40 or 1/10 */
> > > + ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
> > > + if (!ret) {
> > > + DRM_ERROR("Set TMDS ratio failed\n");
> > > + return;
> > > + }
> > > + }
> > > +
> > > + if (config->hdmi_scrambling) {
> > > + /* Enable/disable sink scrambling */
> > > + ret = drm_scdc_set_scrambling(adptr, enable);
> > > + if (!ret) {
> > > + DRM_ERROR("Set sink scrambling failed\n");
> > > + return;
> > > + }
> > > + }
> > > +
> > > + DRM_DEBUG_KMS("sink scrambling handled\n");
> > > +}
> > > +
> > > +uint32_t
> > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > + struct intel_crtc_state *config, uint32_t hdmi_config)
> > > +{
> > > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> > > + struct drm_connector *connector = &intel_hdmi->attached_connector->base;
> > > +
> > > + DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
> > > + intel_encoder->base.name, connector->name);
> >
> > Instead of debugging here, print the hdmi scrambling values in
> > intel_dump_pipe_config().
>
> I am also interested in which encoder and connector are we setting the
> values for, is it that bad :) ?
The debug for the modeset should tell you that, but IMO it would be ok to leave
this extra debug message if the function would still exist.
Ander
> >
> > > + hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING_MASK |
> > > + TRANS_DDI_HIGH_TMDS_CHAR_RATE);
> >
> > This is unnecessary. The hunk below belongs in
> > intel_ddi_enable_transcoder_func() and that function does not do
> > read-modify-
> > write. Any bit that is not explicitly set is zero. I think things are just
> > simpler if this function doesn't exist.
>
> Yeah, will do that.
> > Ander
> >
> > > + if (config->hdmi_scrambling)
> > > + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
> > > +
> > > + if (config->hdmi_high_tmds_clock_ratio)
> > > + hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > +
> > > + return hdmi_config;
> > > +}
> > > +
> > > static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> > > enum port port)
> > > {
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
