>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >Sent: Thursday, October 1, 2015 3:24 PM >To: Shankar, Uma; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma >Subject: Re: [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder support in >CRTC modeset > >On Wed, 30 Sep 2015, Uma Shankar <uma.shankar@xxxxxxxxx> wrote: >> From: Shashank Sharma <shashank.sharma@xxxxxxxxx> >> >> SKL and BXT qualifies the HAS_DDI() check, and hence haswell modeset >> functions are re-used for modeset sequence. But DDI interface doesn't >> include support for DSI. >> This patch adds: >> 1. cases for DSI encoder, in those modeset functions and allows >> a CRTC modeset >> 2. Adds call to pre_pll enabled from CRTC modeset function. Nothing >> needs to be done as such in CRTC for DSI encoder, as PLL, clock >> and and transcoder programming will be taken care in encoder's >> pre_enable and pre_pll_enable function. >> >> v2: Fixed Jani's review comments. Added INVALID_PORT for non DDI >> encoder like DSI for platforms having HAS_DDI as true. >> >> v3: Rebased on latest drm-nightly branch. Added a WARN_ON for invalid >> encoder. >> >> v4: WARN_ON for invalid encoder is refactored as per Jani's suggestion. >> Fixed the sequence for pre_pll_enable. >> >> v5: Protected DDI code paths in case of DSI encoder calls. >> >> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> >> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_ddi.c | 83 ++++++++++++++++++++++++++++- >---- >> drivers/gpu/drm/i915/intel_display.c | 21 ++++++--- >> drivers/gpu/drm/i915/intel_dp_mst.c | 8 +++- >> drivers/gpu/drm/i915/intel_opregion.c | 9 +++- >> 5 files changed, 101 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h index fd1de45..f97a2a2 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2482,6 +2482,7 @@ struct drm_i915_cmd_table { >> INTEL_INFO(dev)->gen >= 9) >> >> #define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi) >> +#define has_encoder_ddi(type) ((type) == (INTEL_OUTPUT_DSI) ? 0 : >> +1) > >Drop this change. > >> #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)- >>has_fpga_dbg) >> #define HAS_PSR(dev) (IS_HASWELL(dev) || >IS_BROADWELL(dev) || \ >> IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) >|| \ diff --git >> a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index cacb07b..4abc13d 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -390,8 +390,10 @@ void intel_prepare_ddi(struct drm_device *dev) >> enum port port; >> bool supports_hdmi; >> >> - ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); >> + if (intel_encoder->type == INTEL_OUTPUT_DSI) >> + continue; >> > >This is good. > >> + ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); >> if (visited[port]) >> continue; >> >> @@ -977,8 +979,16 @@ static void bxt_ddi_clock_get(struct intel_encoder >*encoder, >> struct intel_crtc_state *pipe_config) { >> struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; >> - enum port port = intel_ddi_get_encoder_port(encoder); >> - uint32_t dpll = port; >> + enum port port; >> + uint32_t dpll; >> + >> + if (!has_encoder_ddi(encoder->type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return; >> + } >> + >> + port = intel_ddi_get_encoder_port(encoder); >> + dpll = port; > >No. Drop this change. > >Ask yourself, is it okay to call this function (bxt_ddi_clock_get) if the encoder >type is DSI. > >No, it's not. We should not be here, at all, if encoder is DSI. > >We have modified ddi_get_encoder_port (and therefore >intel_ddi_get_encoder_port) to emit a big warning if that function is called on a >DSI encoder. > >So don't make those extra checks. We'll catch the bugs with the warning in >ddi_get_encoder_port. We'll fix the code paths, and move on. > >We don't care if the function goes on and does something stupid because we >called it with the wrong preconditions. Similarly, we don't check for, say, >IS_GEN2() in DDI code, because that should not have happened. The bug is >somewhere else. > >Now, there *are* valid code paths which will be called if encoder is DSI. In those >cases, we need to handle DSI gracefully and not call ddi_get_encoder_port. For >example intel_prepare_ddi above and intel_opregion_notify_encoder below. > Ok got it. Will do the same. Regards, Uma Shankar >> >> pipe_config->port_clock = >> bxt_calc_pll_link(dev_priv, dpll); >> @@ -1568,10 +1578,16 @@ void intel_ddi_enable_transcoder_func(struct >drm_crtc *crtc) >> struct drm_i915_private *dev_priv = dev->dev_private; >> enum pipe pipe = intel_crtc->pipe; >> enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); >> + enum port port; >> int type = intel_encoder->type; >> uint32_t temp; >> >> + if (!has_encoder_ddi(type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return; >> + } >> + >> + port = intel_ddi_get_encoder_port(intel_encoder); > >Drop this change. > >> /* Enable TRANS_DDI_FUNC_CTL for the pipe to work in HDMI mode */ >> temp = TRANS_DDI_FUNC_ENABLE; >> temp |= TRANS_DDI_SELECT_PORT(port); @@ -1678,12 +1694,18 @@ >bool >> intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_encoder *intel_encoder = intel_connector->encoder; >> int type = intel_connector->base.connector_type; >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); >> + enum port port; >> enum pipe pipe = 0; >> enum transcoder cpu_transcoder; >> enum intel_display_power_domain power_domain; >> uint32_t tmp; >> >> + if (!has_encoder_ddi(type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return false; >> + } >> + >> + port = intel_ddi_get_encoder_port(intel_encoder); > >Drop this change. > >> power_domain = intel_display_port_power_domain(intel_encoder); >> if (!intel_display_power_is_enabled(dev_priv, power_domain)) >> return false; >> @@ -1725,11 +1747,17 @@ bool intel_ddi_get_hw_state(struct >> intel_encoder *encoder, { >> struct drm_device *dev = encoder->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> - enum port port = intel_ddi_get_encoder_port(encoder); >> + enum port port; >> enum intel_display_power_domain power_domain; >> u32 tmp; >> int i; >> >> + if (!has_encoder_ddi(encoder->type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return false; >> + } >> + >> + port = intel_ddi_get_encoder_port(encoder); > >Drop this change. > >> power_domain = intel_display_port_power_domain(encoder); >> if (!intel_display_power_is_enabled(dev_priv, power_domain)) >> return false; >> @@ -1779,11 +1807,18 @@ bool intel_ddi_get_hw_state(struct >> intel_encoder *encoder, void intel_ddi_enable_pipe_clock(struct >> intel_crtc *intel_crtc) { >> struct drm_crtc *crtc = &intel_crtc->base; >> - struct drm_i915_private *dev_priv = crtc->dev->dev_private; >> + struct drm_device *dev = crtc->dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); >> + enum port port; >> enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; >> >> + if (!has_encoder_ddi(intel_encoder->type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return; >> + } >> + >> + port = intel_ddi_get_encoder_port(intel_encoder); > >Drop this change. > >> if (cpu_transcoder != TRANSCODER_EDP) >> I915_WRITE(TRANS_CLK_SEL(cpu_transcoder), >> TRANS_CLK_SEL_PORT(port)); >> @@ -1866,10 +1901,16 @@ static void intel_ddi_pre_enable(struct >intel_encoder *intel_encoder) >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); >> + enum port port; >> int type = intel_encoder->type; >> int hdmi_level; >> >> + if (!has_encoder_ddi(type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return; >> + } >> + >> + port = intel_ddi_get_encoder_port(intel_encoder); > >Drop this change. > >> if (type == INTEL_OUTPUT_EDP) { >> struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> intel_edp_panel_on(intel_dp); >> @@ -1942,11 +1983,17 @@ static void intel_ddi_post_disable(struct >intel_encoder *intel_encoder) >> struct drm_encoder *encoder = &intel_encoder->base; >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); >> + enum port port; >> int type = intel_encoder->type; >> uint32_t val; >> bool wait = false; >> >> + if (!has_encoder_ddi(type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return; >> + } >> + >> + port = intel_ddi_get_encoder_port(intel_encoder); > >Drop this change. > >> val = I915_READ(DDI_BUF_CTL(port)); >> if (val & DDI_BUF_CTL_ENABLE) { >> val &= ~DDI_BUF_CTL_ENABLE; >> @@ -1983,9 +2030,15 @@ static void intel_enable_ddi(struct intel_encoder >*intel_encoder) >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); >> + enum port port; >> int type = intel_encoder->type; >> >> + if (!has_encoder_ddi(type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return; >> + } >> + >> + port = intel_ddi_get_encoder_port(intel_encoder); > >Drop this change. > >> if (type == INTEL_OUTPUT_HDMI) { >> struct intel_digital_port *intel_dig_port = >> enc_to_dig_port(encoder); >> @@ -2729,8 +2782,14 @@ static bool intel_ddi_compute_config(struct >intel_encoder *encoder, >> struct intel_crtc_state *pipe_config) { >> int type = encoder->type; >> - int port = intel_ddi_get_encoder_port(encoder); >> + int port; >> + >> + if (!has_encoder_ddi(type)) { >> + DRM_DEBUG_KMS("DDI func getting called for DSI?\n"); >> + return false; >> + } >> >> + port = intel_ddi_get_encoder_port(encoder); > >Drop this change. > >> WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on >unknown >> output!\n"); >> >> if (port == PORT_A) >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index b8e0310..ea0f533 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4991,6 +4991,7 @@ static void haswell_crtc_enable(struct drm_crtc >*crtc) >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct intel_encoder *encoder; >> + bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI); >> int pipe = intel_crtc->pipe; >> >> WARN_ON(!crtc->state->enable); >> @@ -5023,9 +5024,12 @@ static void haswell_crtc_enable(struct drm_crtc >*crtc) >> intel_crtc->active = true; >> >> intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); >> - for_each_encoder_on_crtc(dev, crtc, encoder) >> + for_each_encoder_on_crtc(dev, crtc, encoder) { >> + if (encoder->pre_pll_enable) >> + encoder->pre_pll_enable(encoder); >> if (encoder->pre_enable) >> encoder->pre_enable(encoder); >> + } >> >> if (intel_crtc->config->has_pch_encoder) { >> intel_set_pch_fifo_underrun_reporting(dev_priv, >TRANSCODER_A, @@ >> -5033,7 +5037,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) >> dev_priv->display.fdi_link_train(crtc); >> } >> >> - intel_ddi_enable_pipe_clock(intel_crtc); >> + if (!is_dsi) >> + intel_ddi_enable_pipe_clock(intel_crtc); >> >> if (INTEL_INFO(dev)->gen == 9) >> skylake_pfit_update(intel_crtc, 1); @@ -5049,7 +5054,8 @@ >static >> void haswell_crtc_enable(struct drm_crtc *crtc) >> intel_crtc_load_lut(crtc); >> >> intel_ddi_set_pipe_settings(crtc); >> - intel_ddi_enable_transcoder_func(crtc); >> + if (!is_dsi) >> + intel_ddi_enable_transcoder_func(crtc); >> >> intel_update_watermarks(crtc); >> intel_enable_pipe(intel_crtc); >> @@ -5057,7 +5063,7 @@ static void haswell_crtc_enable(struct drm_crtc >*crtc) >> if (intel_crtc->config->has_pch_encoder) >> lpt_pch_enable(crtc); >> >> - if (intel_crtc->config->dp_encoder_is_mst) >> + if (intel_crtc->config->dp_encoder_is_mst && !is_dsi) >> intel_ddi_set_vc_payload_alloc(crtc, true); >> >> assert_vblank_disabled(crtc); >> @@ -5159,6 +5165,7 @@ static void haswell_crtc_disable(struct drm_crtc >*crtc) >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct intel_encoder *encoder; >> enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; >> + bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI); >> >> if (!intel_crtc->active) >> return; >> @@ -5179,7 +5186,8 @@ static void haswell_crtc_disable(struct drm_crtc >*crtc) >> if (intel_crtc->config->dp_encoder_is_mst) >> intel_ddi_set_vc_payload_alloc(crtc, false); >> >> - intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder); >> + if (!is_dsi) >> + intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder); >> >> if (INTEL_INFO(dev)->gen == 9) >> skylake_pfit_update(intel_crtc, 0); @@ -5188,7 +5196,8 @@ >static >> void haswell_crtc_disable(struct drm_crtc *crtc) >> else >> MISSING_CASE(INTEL_INFO(dev)->gen); >> >> - intel_ddi_disable_pipe_clock(intel_crtc); >> + if (!is_dsi) >> + intel_ddi_disable_pipe_clock(intel_crtc); >> >> if (intel_crtc->config->has_pch_encoder) { >> lpt_disable_pch_transcoder(dev_priv); >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c >> b/drivers/gpu/drm/i915/intel_dp_mst.c >> index 600afdb..00327bc 100644 >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c >> @@ -172,8 +172,14 @@ static void intel_mst_pre_enable_dp(struct >intel_encoder *encoder) >> intel_mst->port = found->port; >> >> if (intel_dp->active_mst_links == 0) { >> - enum port port = intel_ddi_get_encoder_port(encoder); >> + enum port port; >> >> + if (!has_encoder_ddi(encoder->type)) { >> + DRM_DEBUG_KMS("DDI func getting called for >DSI?\n"); >> + return; >> + } >> + >> + port = intel_ddi_get_encoder_port(encoder); > >Drop this change. > >> /* FIXME: add support for SKL */ >> if (INTEL_INFO(dev)->gen < 9) >> I915_WRITE(PORT_CLK_SEL(port), >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c >> b/drivers/gpu/drm/i915/intel_opregion.c >> index 4813374..64b8bcd 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -334,8 +334,12 @@ int intel_opregion_notify_encoder(struct >intel_encoder *intel_encoder, >> if (!HAS_DDI(dev)) >> return 0; >> >> - port = intel_ddi_get_encoder_port(intel_encoder); >> - if (port == PORT_E) { >> + if (!has_encoder_ddi(type)) > >This is good, but don't add extra wrapper (has_encoder_ddi) for the encode type >check. > >> + port = 0; >> + else >> + port = intel_ddi_get_encoder_port(intel_encoder); >> + >> + if (port == PORT_E) { >> port = 0; >> } else { >> parm |= 1 << port; >> @@ -356,6 +360,7 @@ int intel_opregion_notify_encoder(struct >intel_encoder *intel_encoder, >> type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL; >> break; >> case INTEL_OUTPUT_EDP: >> + case INTEL_OUTPUT_DSI: >> type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL; >> break; >> default: >> -- >> 1.7.9.5 >> > >-- >Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
