[PATCH v4 5/6] drm/i915: enable scrambling
Sharma, Shashank
shashank.sharma at intel.com
Thu Feb 23 11:03:10 UTC 2017
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 at intel.com>
>>>> ---
>>>> 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.
Do you still think it's worth doing it ?
> 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
Think about two situations where:-
- Monitor supports scrambling and scdc, but we will not enable it, as
the current mode is 1080P at 148 MHz
- Monitor supports scrambling and scdc, and we will enable it, as the
current mode is 4k at 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 ?
- Shashank
> Ander
>
>
>> - Shashank
>>>> +
>>>> static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>>>> enum port port)
>>>> {
>>>> --
>>>> 1.9.1
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list