On Thu, Feb 23, 2017 at 03:07:40PM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2017-02-23 at 16:33 +0530, Sharma, Shashank wrote:
> > Thanks for the review Ander, my comments, inline.
> >
> >
> > Regards
> >
> > Shashank
> >
> >
> > On 2/23/2017 1:33 PM, Ander Conselvan De Oliveira wrote:
> > > On Thu, 2017-02-23 at 10:01 +0530, Sharma, Shashank wrote:
> > > > Regards
> > > >
> > > > Shashank
> > > >
> > > >
> > > > On 2/22/2017 10:59 PM, Ville Syrjälä wrote:
> > > > > On Wed, Feb 22, 2017 at 06:48:30PM +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
> > > > > >
> > > > > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 4 ++
> > > > > > drivers/gpu/drm/i915/intel_ddi.c | 28 ++++++++++
> > > > > > drivers/gpu/drm/i915/intel_drv.h | 14 +++++
> > > > > > drivers/gpu/drm/i915/intel_hdmi.c | 108 ++++++++++++++++++++++++++++++++++++++
> > > > > > 4 files changed, 154 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 141a5c1..81cf10b 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -7819,7 +7819,11 @@ 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)
> > > > > >
> > > > > > /* 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 cd6fedd..bd8293d 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_handle_source_scrambling(
> > > > > > + intel_encoder,
> > > > > > + &intel_crtc->config->base.adjusted_mode, temp);
> > > > > > } else if (type == INTEL_OUTPUT_ANALOG) {
> > > > > > temp |= TRANS_DDI_MODE_SELECT_FDI;
> > > > > > temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> > > > > > @@ -1843,6 +1848,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);
> > > > > > + }
> > > > > > +
> > > > > > /* In HDMI/DVI mode, the port width, and swing/emphasis values
> > > > > > * are ignored so nothing special needs to be done besides
> > > > > > * enabling the port.
> > > > > > @@ -1875,6 +1895,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);
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 821c57c..c7262d7 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -691,6 +691,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 {
> > > > > > @@ -1609,6 +1615,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 drm_display_mode *mode,
> > > > > > + uint32_t 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 a580de8..c44beee 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>
> > > > > > @@ -1795,6 +1796,113 @@ 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_scdc *scdc = &connector->display_info.hdmi.scdc;
> > > > > > + struct drm_scrambling *scrambling = &scdc->scrambling;
> > > > > > + struct drm_display_mode *mode = &config->base.adjusted_mode;
> > > > > > + struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > > > > > + struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> > >
> > > There's only two letters missing. Maybe just spell adapter.
> >
> > The intention was to keep it within 80 char, and it was going 82 :P :-)
> > > > > > + intel_hdmi->ddc_bus);
> > > > > > +
> > > > > > + if (!scrambling->supported)
> > > > > > + return;
> > > > > > +
> > > > > > + DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> > > > > > + intel_encoder->base.name, connector->name);
> > > > > > +
> > > > > > + if (enable) {
> > > > > > +
> > > > > > + if (mode->clock > 340000) {
> > > > > > + /* Set TMDS bit clock ratio to 1/40 */
> > > > > > + config->hdmi_high_tmds_clock_ratio =
> > > > > > + drm_scdc_set_high_tmds_clock_ratio(adptr);
> > > > >
> > > > > You're not allowed to muck with the state anymore. All state computation
> > > > > should happen in the .compute_config hook.
> > > >
> > > > ok, let me re-arrange this function in such a way that I do the config
> > > > update part in compute_config
> > > > > > + if (!config->hdmi_high_tmds_clock_ratio) {
> > > > > > + DRM_ERROR("Set high TMDS ratio failed\n");
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + /* Enable sink scrambling */
> > > > > > + config->hdmi_scrambling =
> > > > > > + drm_scdc_enable_scrambling(adptr);
> > > > > > + if (!config->hdmi_scrambling) {
> > > > > > + DRM_ERROR("Can't enable sink scrambling\n");
> > > > > > + return;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + /* Few sinks support scrambling at clocks <=340 MHz too */
> > > > > > + if (!config->hdmi_scrambling && scrambling->low_rates) {
> > > > > > + config->hdmi_scrambling =
> > > > > > + drm_scdc_enable_scrambling(adptr);
> > > > > > + if (!config->hdmi_scrambling)
> > > > > > + DRM_ERROR("Can't enable sink scrambling\n");
> > > > > > + }
> > > > > > +
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + if (config->hdmi_high_tmds_clock_ratio) {
> > > > > > + /* Set TMDS bit clock ratio back to 1/10 */
> > > > > > + config->hdmi_high_tmds_clock_ratio =
> > > > > > + !(drm_scdc_clear_high_tmds_clock_ratio(adptr));
> > > > > > + if (config->hdmi_high_tmds_clock_ratio)
> > > > > > + DRM_ERROR("Reset high TMDS ratio failed\n");
> > > > > > + }
> > > > > > +
> > > > > > + if (config->hdmi_scrambling) {
> > > > > > + /* Disable sink scrambling */
> > > > > > + config->hdmi_scrambling = !(drm_scdc_disable_scrambling(adptr));
> > > > > > + if (config->hdmi_scrambling)
> > > > > > + DRM_ERROR("Disable sink scrambling failed\n");
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +static inline uint32_t _intel_hdmi_set_source_scrambling(uint32_t hdmi_config)
> > > > > > +{
> > > > > > + return hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
> > > > > > + TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > > > > + TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > > > > +}
> > > > > > +
> > > > > > +uint32_t
> > > > > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > > > > + struct drm_display_mode *mode, 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;
> > > > > > + struct drm_hdmi_info *hdmi_info = &connector->display_info.hdmi;
> > > > > > + struct drm_scrambling *scrambling = &hdmi_info->scdc.scrambling;
> > > > > > +
> > > > > > + DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
> > > > > > + intel_encoder->base.name, connector->name);
> > > > > > +
> > > > > > + hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
> > > > > > + TRANS_DDI_HIGH_TMDS_CHAR_RATE |
> > > > > > + TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > > > > + TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > > > > +
> > > > > > + if (mode->clock <= 340000) {
> > > > > > + /* Few sinks support scrambling at rate < 340 MHz too */
> > > > > > + if (scrambling->low_rates)
> > > > > > + hdmi_config =
> > > > > > + _intel_hdmi_set_source_scrambling(hdmi_config);
> > > > > > + return hdmi_config;
> > > > > > + }
> > > > > > +
> > > > > > + /* Scrambling or not, if clock > 340 MHz, set high char rate */
> > > > > > + hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > > > > +
> > > > > > + if (scrambling->supported)
> > > > > > + hdmi_config = _intel_hdmi_set_source_scrambling(hdmi_config);
> > > > > > +
> > > > > > + return hdmi_config;
> > > > > > +}
> > > > >
> > > > > Seems overly complicated to me. It could just be something simple like:
> > > > >
> > > > > void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> > > > > {
> > > > > ...
> > > > > if (config->hdmi_scrambling)
> > > > > temp |= TRANS_DDI_HDMI_SCRAMBLING;
> > > > >
> > > > > if (config->whatever)
> > > > > temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > > > > ...
> > > > > }
> > > >
> > > > I know it appears like complicated, but actually the requirement is
> > > > complicated:
> > > > - if clock is > 340 Mhz, set scrambling && set_high_tmds_clock_ratio
> > > > - if clock is < 340 Mhz, but if monitor supports scrambling at lower
> > > > clocks, set scrambling only (not tmds_clock_ratio)
> > > > - to set scrambling, we have to first check if scrambling is supported
> > > > in monitor
> > > > So to meet all these condition, you will end up with something like
> > > > what's implemented above.
> > > > Do you see a better way ?
> > >
> > > Put that intel_hdmi_compute_config() so you check it only once, i.e.:
> > >
> > > if (clock > 340 || scrambling->low_rates)
> > > config->hdmi_scrambling = true;
> > >
> > > if (clock > 340)
> > > config->hdmi_high_tmds = true;
> > >
> > > Then you can do just as Ville said above in intel_ddi_enable_transcoder_func().
> >
> > Thats a good idea, I can do that, but there is one drawback of doing
> > that. Right now, all of the HDMI
> > source/sink scrambling code is in one function, so for any
> > debug/modification activity, you have to look
> > only in one function, also its more readable this way (in the flow)
> >
> > The moment we distribute code in 3 different function, you have to track
> > in all the places. Also, we are not
> > saving/reducing any if conditions, we are just moving it at some other
> > place.
>
> With the current patch both handle_source_scrambling() and
> handle_sink_scrambling() have checks for clock above/below 340000 and whether
> the sink support scrambling with low rates. Those two checks don't need to be
> duplicated.
>
> > Do you still think it's worth doing it ?
>
> Yes, doing it in compute config follows the convention of the driver. If
> something is set in crtc_state, it is only natural to assume it was set at some
> point in the atomic check phase or from reading out the hardware state.
>
> > > And also for the sink part, no need to check the input values again:
> > >
> > > if (config->hdmi_high_tmds)
> > > if (!drm_scdc_set_high_tmds_clock_ratio(adapter, enable))
> > > DRM_ERROR();
> > >
> > > if (config->hdmi_scrambing)
> > > if (!drm_scdc_set_scrambling(adapter, enable))
> > > DRM_ERROR();
> > >
> > > At least that's how I'd interpret what Ville wrote.
> >
> > Actually design is slightly different. The state's hdmi_scrambling/clock
> > bool's indicate that the sink's
> > scrambling/high_clock_ratio is enabled/set by source (and needs to be
> > disabled), whereas connecotr's->display_info->scdc.scrambling
> > is to indicate monitor's capability to support scrambling and scdc
>
> The crtc_state shouldn't be changed to represent changes in the hardware state.
> It is computed during the atomic check phase, and after that it should represent
> the state that needs to be commited. Perhaps the code in hdmi_compute_config()
> just needs to take the previous state into account?
The previous state is irrelevant. We just need to compute these things
based on what we're going to do next. And this stuff doesn't really
track the state of the sink, rather it tracks the state of the source.
The sink state just follows along to match. So in the readout path we
just want to read out the state of the source.
>
> >
> > Think about two situations where:-
> > - Monitor supports scrambling and scdc, but we will not enable it, as
> > the current mode is 1080P@148 MHz
> > - Monitor supports scrambling and scdc, and we will enable it, as the
> > current mode is 4k@596 Mhz
> >
> > To differentiate between these two, we have:
> > config->hdmi_scrambling which shows scrambling enabled on monitor by source
> > display_info->hdmi.scdc->scrambling which indicates monitor supports
> > scrambling
> >
> > Does it make sense ? Or you prefer some changes here ?
>
> Is there any harm in disabling scrambling twice? If not I'd say just disable it
> on every modeset if it is not needed. Then there's no need to know the previous
> state at the moment we actually enable/disable it.
Agreed. Just explicitly set the state to what we want every time.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
