| Message ID | 1485953081-7630-6-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote: > Geminilake platform has 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. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > drivers/gpu/drm/i915/intel_ddi.c | 5 +++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 51 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 495b789..cc85892 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7807,6 +7807,8 @@ enum { > #define TRANS_DDI_EDP_INPUT_C_ONOFF (6<<12) > #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1<<8) > #define TRANS_DDI_BFI_ENABLE (1<<4) > +#define TRANS_DDI_HIGH_TMDS_CHAR_RATE (1<<4) > +#define TRANS_DDI_HDMI_SCRAMBLING (1<<0) > > /* 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 9a9a670..aea81ce 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1278,6 +1278,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_check_scrambling(intel_encoder, > + &intel_crtc->config->base.adjusted_mode); > + } > } else if (type == INTEL_OUTPUT_ANALOG) { > temp |= TRANS_DDI_MODE_SELECT_FDI; > temp |= (intel_crtc->config->fdi_lanes - 1) << 1; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 393f243..aafce7f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1588,6 +1588,8 @@ 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_check_scrambling(struct intel_encoder *intel_encoder, > + struct drm_display_mode *mode); > 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 ebae2bd..92dd9bc 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector) > intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > } > > +static void > +intel_hdmi_enable_scrambling(struct drm_connector *connector) > +{ > + struct drm_i915_private *dev_priv = connector->dev->dev_private; > + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > + struct i2c_adapter *adapter = > + intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); > + > + if (!drm_enable_scrambling(connector, adapter, true)) > + DRM_ERROR("Request to enable scrambling failed\n"); > +} I don't like hiding this somewhere deep like this. It should be somewhere much higher up. And I'm thinkign we might want to track the scrambler state in the crtc state. > + > +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder, > + struct drm_display_mode *mode) > +{ > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base); > + struct drm_connector *connector = &intel_hdmi->attached_connector->base; > + struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info; > + uint32_t hdmi_config = 0; > + > + DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n", > + intel_encoder->base.name, connector->name); > + > + if (mode->clock < 340000) { > + if (hdmi_info->scr_info.low_clocks) { > + intel_hdmi_enable_scrambling(connector); > + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; > + } > + return hdmi_config; > + } > + > + /* Enable scrambling for clocks > 340M */ > + if (hdmi_info->scr_info.supported) { > + intel_hdmi_enable_scrambling(connector); > + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; > + } > + > + /* Scrambling or not, if clock > 340M, set high char rate */ > + 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) > { > -- > 1.9.1
Regards Shashank On 2/1/2017 10:06 PM, Ville Syrjälä wrote: > On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote: >> Geminilake platform has 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. >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 2 ++ >> drivers/gpu/drm/i915/intel_ddi.c | 5 +++++ >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 51 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 495b789..cc85892 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -7807,6 +7807,8 @@ enum { >> #define TRANS_DDI_EDP_INPUT_C_ONOFF (6<<12) >> #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1<<8) >> #define TRANS_DDI_BFI_ENABLE (1<<4) >> +#define TRANS_DDI_HIGH_TMDS_CHAR_RATE (1<<4) >> +#define TRANS_DDI_HDMI_SCRAMBLING (1<<0) >> >> /* 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 9a9a670..aea81ce 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1278,6 +1278,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_check_scrambling(intel_encoder, >> + &intel_crtc->config->base.adjusted_mode); >> + } >> } else if (type == INTEL_OUTPUT_ANALOG) { >> temp |= TRANS_DDI_MODE_SELECT_FDI; >> temp |= (intel_crtc->config->fdi_lanes - 1) << 1; >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 393f243..aafce7f 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1588,6 +1588,8 @@ 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_check_scrambling(struct intel_encoder *intel_encoder, >> + struct drm_display_mode *mode); >> 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 ebae2bd..92dd9bc 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector) >> intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >> } >> >> +static void >> +intel_hdmi_enable_scrambling(struct drm_connector *connector) >> +{ >> + struct drm_i915_private *dev_priv = connector->dev->dev_private; >> + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); >> + struct i2c_adapter *adapter = >> + intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); >> + >> + if (!drm_enable_scrambling(connector, adapter, true)) >> + DRM_ERROR("Request to enable scrambling failed\n"); >> +} > I don't like hiding this somewhere deep like this. It should be > somewhere much higher up. Why ? All we need to do here is enable two bits in transcoder control register, which is already being programmed in a calling function, so I dont see the use case, but I might be missing some bigger picture. Can you please elaborate on this ? > > And I'm thinkign we might want to track the scrambler state > in the crtc state. Yes, that's a pretty good way to track dynamic status of scrambler, do you think we should add this in drm_crtc_state itself ? - Shashank >> + >> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder, >> + struct drm_display_mode *mode) >> +{ >> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base); >> + struct drm_connector *connector = &intel_hdmi->attached_connector->base; >> + struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info; >> + uint32_t hdmi_config = 0; >> + >> + DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n", >> + intel_encoder->base.name, connector->name); >> + >> + if (mode->clock < 340000) { >> + if (hdmi_info->scr_info.low_clocks) { >> + intel_hdmi_enable_scrambling(connector); >> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; >> + } >> + return hdmi_config; >> + } >> + >> + /* Enable scrambling for clocks > 340M */ >> + if (hdmi_info->scr_info.supported) { >> + intel_hdmi_enable_scrambling(connector); >> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; >> + } >> + >> + /* Scrambling or not, if clock > 340M, set high char rate */ >> + 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) >> { >> -- >> 1.9.1
On Thu, Feb 02, 2017 at 11:23:19AM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 2/1/2017 10:06 PM, Ville Syrjälä wrote: > > On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote: > >> Geminilake platform has 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. > >> > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_reg.h | 2 ++ > >> drivers/gpu/drm/i915/intel_ddi.c | 5 +++++ > >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > >> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++ > >> 4 files changed, 51 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 495b789..cc85892 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -7807,6 +7807,8 @@ enum { > >> #define TRANS_DDI_EDP_INPUT_C_ONOFF (6<<12) > >> #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1<<8) > >> #define TRANS_DDI_BFI_ENABLE (1<<4) > >> +#define TRANS_DDI_HIGH_TMDS_CHAR_RATE (1<<4) > >> +#define TRANS_DDI_HDMI_SCRAMBLING (1<<0) > >> > >> /* 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 9a9a670..aea81ce 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -1278,6 +1278,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_check_scrambling(intel_encoder, > >> + &intel_crtc->config->base.adjusted_mode); > >> + } > >> } else if (type == INTEL_OUTPUT_ANALOG) { > >> temp |= TRANS_DDI_MODE_SELECT_FDI; > >> temp |= (intel_crtc->config->fdi_lanes - 1) << 1; > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index 393f243..aafce7f 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -1588,6 +1588,8 @@ 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_check_scrambling(struct intel_encoder *intel_encoder, > >> + struct drm_display_mode *mode); > >> 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 ebae2bd..92dd9bc 100644 > >> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector) > >> intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >> } > >> > >> +static void > >> +intel_hdmi_enable_scrambling(struct drm_connector *connector) > >> +{ > >> + struct drm_i915_private *dev_priv = connector->dev->dev_private; > >> + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > >> + struct i2c_adapter *adapter = > >> + intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); > >> + > >> + if (!drm_enable_scrambling(connector, adapter, true)) > >> + DRM_ERROR("Request to enable scrambling failed\n"); > >> +} > > I don't like hiding this somewhere deep like this. It should be > > somewhere much higher up. > Why ? All we need to do here is enable two bits in transcoder control > register, which is already being > programmed in a calling function, so I dont see the use case, but I > might be missing some bigger picture. > Can you please elaborate on this ? We're talking to the display here, which is rather surprising to happen from a function thing that on the first glance doesn't even seem to touch the hardware. > > > > And I'm thinkign we might want to track the scrambler state > > in the crtc state. > Yes, that's a pretty good way to track dynamic status of scrambler, do > you think we should add this in > drm_crtc_state itself ? I'd just start with intel specific state and later we can see what everyone else wants to do and potentially unify a bit. > > - Shashank > >> + > >> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder, > >> + struct drm_display_mode *mode) > >> +{ > >> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base); > >> + struct drm_connector *connector = &intel_hdmi->attached_connector->base; > >> + struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info; > >> + uint32_t hdmi_config = 0; > >> + > >> + DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n", > >> + intel_encoder->base.name, connector->name); > >> + > >> + if (mode->clock < 340000) { > >> + if (hdmi_info->scr_info.low_clocks) { > >> + intel_hdmi_enable_scrambling(connector); > >> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; > >> + } > >> + return hdmi_config; > >> + } > >> + > >> + /* Enable scrambling for clocks > 340M */ > >> + if (hdmi_info->scr_info.supported) { > >> + intel_hdmi_enable_scrambling(connector); > >> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; > >> + } > >> + > >> + /* Scrambling or not, if clock > 340M, set high char rate */ > >> + 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) > >> { > >> -- > >> 1.9.1
Regards Shashank On 2/2/2017 3:32 PM, Ville Syrjälä wrote: > On Thu, Feb 02, 2017 at 11:23:19AM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 2/1/2017 10:06 PM, Ville Syrjälä wrote: >>> On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote: >>>> Geminilake platform has 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. >>>> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++ >>>> drivers/gpu/drm/i915/intel_ddi.c | 5 +++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >>>> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 51 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index 495b789..cc85892 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -7807,6 +7807,8 @@ enum { >>>> #define TRANS_DDI_EDP_INPUT_C_ONOFF (6<<12) >>>> #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1<<8) >>>> #define TRANS_DDI_BFI_ENABLE (1<<4) >>>> +#define TRANS_DDI_HIGH_TMDS_CHAR_RATE (1<<4) >>>> +#define TRANS_DDI_HDMI_SCRAMBLING (1<<0) >>>> >>>> /* 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 9a9a670..aea81ce 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>> @@ -1278,6 +1278,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_check_scrambling(intel_encoder, >>>> + &intel_crtc->config->base.adjusted_mode); >>>> + } >>>> } else if (type == INTEL_OUTPUT_ANALOG) { >>>> temp |= TRANS_DDI_MODE_SELECT_FDI; >>>> temp |= (intel_crtc->config->fdi_lanes - 1) << 1; >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>> index 393f243..aafce7f 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -1588,6 +1588,8 @@ 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_check_scrambling(struct intel_encoder *intel_encoder, >>>> + struct drm_display_mode *mode); >>>> 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 ebae2bd..92dd9bc 100644 >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector) >>>> intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>> } >>>> >>>> +static void >>>> +intel_hdmi_enable_scrambling(struct drm_connector *connector) >>>> +{ >>>> + struct drm_i915_private *dev_priv = connector->dev->dev_private; >>>> + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); >>>> + struct i2c_adapter *adapter = >>>> + intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); >>>> + >>>> + if (!drm_enable_scrambling(connector, adapter, true)) >>>> + DRM_ERROR("Request to enable scrambling failed\n"); >>>> +} >>> I don't like hiding this somewhere deep like this. It should be >>> somewhere much higher up. >> Why ? All we need to do here is enable two bits in transcoder control >> register, which is already being >> programmed in a calling function, so I dont see the use case, but I >> might be missing some bigger picture. >> Can you please elaborate on this ? > We're talking to the display here, which is rather surprising to happen > from a function thing that on the first glance doesn't even seem to > touch the hardware. I kind of agree with this, and I am also tempted now to shift this call upwards, but I want to keep this close to port enabling as well, as the HDMI 2.0 spec says: "Max time period between a source enables scrambling on monitor, and starts sending scrambled video should be 100ms" else the sink might choose to disable scrambling. Considering this, where do you think would be right place to enabled scrambling on monitor ? - Shashank >>> And I'm thinkign we might want to track the scrambler state >>> in the crtc state. >> Yes, that's a pretty good way to track dynamic status of scrambler, do >> you think we should add this in >> drm_crtc_state itself ? > I'd just start with intel specific state and later we can see what > everyone else wants to do and potentially unify a bit. > >> - Shashank >>>> + >>>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder, >>>> + struct drm_display_mode *mode) >>>> +{ >>>> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base); >>>> + struct drm_connector *connector = &intel_hdmi->attached_connector->base; >>>> + struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info; >>>> + uint32_t hdmi_config = 0; >>>> + >>>> + DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n", >>>> + intel_encoder->base.name, connector->name); >>>> + >>>> + if (mode->clock < 340000) { >>>> + if (hdmi_info->scr_info.low_clocks) { >>>> + intel_hdmi_enable_scrambling(connector); >>>> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; >>>> + } >>>> + return hdmi_config; >>>> + } >>>> + >>>> + /* Enable scrambling for clocks > 340M */ >>>> + if (hdmi_info->scr_info.supported) { >>>> + intel_hdmi_enable_scrambling(connector); >>>> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; >>>> + } >>>> + >>>> + /* Scrambling or not, if clock > 340M, set high char rate */ >>>> + 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) >>>> { >>>> -- >>>> 1.9.1
On Thu, Feb 02, 2017 at 04:15:21PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 2/2/2017 3:32 PM, Ville Syrjälä wrote: > > On Thu, Feb 02, 2017 at 11:23:19AM +0530, Sharma, Shashank wrote: > >> Regards > >> > >> Shashank > >> > >> > >> On 2/1/2017 10:06 PM, Ville Syrjälä wrote: > >>> On Wed, Feb 01, 2017 at 06:14:40PM +0530, Shashank Sharma wrote: > >>>> Geminilake platform has 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. > >>>> > >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++ > >>>> drivers/gpu/drm/i915/intel_ddi.c | 5 +++++ > >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > >>>> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++ > >>>> 4 files changed, 51 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >>>> index 495b789..cc85892 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_reg.h > >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h > >>>> @@ -7807,6 +7807,8 @@ enum { > >>>> #define TRANS_DDI_EDP_INPUT_C_ONOFF (6<<12) > >>>> #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1<<8) > >>>> #define TRANS_DDI_BFI_ENABLE (1<<4) > >>>> +#define TRANS_DDI_HIGH_TMDS_CHAR_RATE (1<<4) > >>>> +#define TRANS_DDI_HDMI_SCRAMBLING (1<<0) > >>>> > >>>> /* 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 9a9a670..aea81ce 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c > >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >>>> @@ -1278,6 +1278,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_check_scrambling(intel_encoder, > >>>> + &intel_crtc->config->base.adjusted_mode); > >>>> + } > >>>> } else if (type == INTEL_OUTPUT_ANALOG) { > >>>> temp |= TRANS_DDI_MODE_SELECT_FDI; > >>>> temp |= (intel_crtc->config->fdi_lanes - 1) << 1; > >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >>>> index 393f243..aafce7f 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_drv.h > >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h > >>>> @@ -1588,6 +1588,8 @@ 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_check_scrambling(struct intel_encoder *intel_encoder, > >>>> + struct drm_display_mode *mode); > >>>> 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 ebae2bd..92dd9bc 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >>>> @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector) > >>>> intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >>>> } > >>>> > >>>> +static void > >>>> +intel_hdmi_enable_scrambling(struct drm_connector *connector) > >>>> +{ > >>>> + struct drm_i915_private *dev_priv = connector->dev->dev_private; > >>>> + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > >>>> + struct i2c_adapter *adapter = > >>>> + intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); > >>>> + > >>>> + if (!drm_enable_scrambling(connector, adapter, true)) > >>>> + DRM_ERROR("Request to enable scrambling failed\n"); > >>>> +} > >>> I don't like hiding this somewhere deep like this. It should be > >>> somewhere much higher up. > >> Why ? All we need to do here is enable two bits in transcoder control > >> register, which is already being > >> programmed in a calling function, so I dont see the use case, but I > >> might be missing some bigger picture. > >> Can you please elaborate on this ? > > We're talking to the display here, which is rather surprising to happen > > from a function thing that on the first glance doesn't even seem to > > touch the hardware. > I kind of agree with this, and I am also tempted now to shift this call > upwards, but I want to > keep this close to port enabling as well, as the HDMI 2.0 spec says: > "Max time period between a source enables scrambling on monitor, and > starts sending scrambled > video should be 100ms" else the sink might choose to disable scrambling. > > Considering this, where do you think would be right place to enabled > scrambling on monitor ? intel_enable_ddi() is where we presumbly enable the port, so maybe there. > > - Shashank > >>> And I'm thinkign we might want to track the scrambler state > >>> in the crtc state. > >> Yes, that's a pretty good way to track dynamic status of scrambler, do > >> you think we should add this in > >> drm_crtc_state itself ? > > I'd just start with intel specific state and later we can see what > > everyone else wants to do and potentially unify a bit. > > > >> - Shashank > >>>> + > >>>> +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder, > >>>> + struct drm_display_mode *mode) > >>>> +{ > >>>> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base); > >>>> + struct drm_connector *connector = &intel_hdmi->attached_connector->base; > >>>> + struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info; > >>>> + uint32_t hdmi_config = 0; > >>>> + > >>>> + DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n", > >>>> + intel_encoder->base.name, connector->name); > >>>> + > >>>> + if (mode->clock < 340000) { > >>>> + if (hdmi_info->scr_info.low_clocks) { > >>>> + intel_hdmi_enable_scrambling(connector); > >>>> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; > >>>> + } > >>>> + return hdmi_config; > >>>> + } > >>>> + > >>>> + /* Enable scrambling for clocks > 340M */ > >>>> + if (hdmi_info->scr_info.supported) { > >>>> + intel_hdmi_enable_scrambling(connector); > >>>> + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; > >>>> + } > >>>> + > >>>> + /* Scrambling or not, if clock > 340M, set high char rate */ > >>>> + 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) > >>>> { > >>>> -- > >>>> 1.9.1
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 495b789..cc85892 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7807,6 +7807,8 @@ enum { #define TRANS_DDI_EDP_INPUT_C_ONOFF (6<<12) #define TRANS_DDI_DP_VC_PAYLOAD_ALLOC (1<<8) #define TRANS_DDI_BFI_ENABLE (1<<4) +#define TRANS_DDI_HIGH_TMDS_CHAR_RATE (1<<4) +#define TRANS_DDI_HDMI_SCRAMBLING (1<<0) /* 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 9a9a670..aea81ce 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1278,6 +1278,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_check_scrambling(intel_encoder, + &intel_crtc->config->base.adjusted_mode); + } } else if (type == INTEL_OUTPUT_ANALOG) { temp |= TRANS_DDI_MODE_SELECT_FDI; temp |= (intel_crtc->config->fdi_lanes - 1) << 1; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 393f243..aafce7f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1588,6 +1588,8 @@ 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_check_scrambling(struct intel_encoder *intel_encoder, + struct drm_display_mode *mode); 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 ebae2bd..92dd9bc 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1795,6 +1795,48 @@ static void intel_hdmi_destroy(struct drm_connector *connector) intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE; } +static void +intel_hdmi_enable_scrambling(struct drm_connector *connector) +{ + struct drm_i915_private *dev_priv = connector->dev->dev_private; + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); + struct i2c_adapter *adapter = + intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); + + if (!drm_enable_scrambling(connector, adapter, true)) + DRM_ERROR("Request to enable scrambling failed\n"); +} + +uint32_t intel_hdmi_check_scrambling(struct intel_encoder *intel_encoder, + struct drm_display_mode *mode) +{ + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base); + struct drm_connector *connector = &intel_hdmi->attached_connector->base; + struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi_info; + uint32_t hdmi_config = 0; + + DRM_DEBUG_KMS("Checking scrambling for enc:%s connector:%s\n", + intel_encoder->base.name, connector->name); + + if (mode->clock < 340000) { + if (hdmi_info->scr_info.low_clocks) { + intel_hdmi_enable_scrambling(connector); + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; + } + return hdmi_config; + } + + /* Enable scrambling for clocks > 340M */ + if (hdmi_info->scr_info.supported) { + intel_hdmi_enable_scrambling(connector); + hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING; + } + + /* Scrambling or not, if clock > 340M, set high char rate */ + 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) {
Geminilake platform has 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. Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/intel_ddi.c | 5 +++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+)