| Message ID | 1443718429-32272-1-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 01 Oct 2015, Uma Shankar <uma.shankar@intel.com> wrote: > From: Shashank Sharma <shashank.sharma@intel.com> > > 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@intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> Reviewed-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 7 +++++-- > drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------ > drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++-- > 3 files changed, 27 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index cacb07b..7b7f544 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; > > + ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); > if (visited[port]) > continue; > > @@ -1779,7 +1781,8 @@ 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 transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; > 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_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > index 4813374..db518ef 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 (intel_encoder->type == INTEL_OUTPUT_DSI) > + 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 >
On Thu, Oct 01, 2015 at 10:23:49PM +0530, Uma Shankar wrote: > From: Shashank Sharma <shashank.sharma@intel.com> > > 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@intel.com> > Signed-off-by: Uma Shankar <uma.shankar@intel.com> Ok, after this patch we get stuff like this: 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, true); dev_priv->display.fdi_link_train(crtc); } if (!is_dsi) intel_ddi_enable_pipe_clock(intel_crtc); 1. Please remove pre_pll_enable again, we don't need 2 callbacks in exactly the same spot. Yes this might mean that you need special bxt_ versions of that in the dsi encoder, we have that everywhere. 2. the has_pch_encoder is already something encoder-specific (it's exclusively used by the HSW LPT CRT encoder). Now we have another one of those for the !is_dsi case. These special-cases should be moved into the encoder->pre_enable callbacks, that's what they're for. I'm not going to block these patches are (18months is already ridiculous), but I want this cleanup done. Uma, can you pls own this? If you can't do it yourself please escalate to Indranil so he can find someone. Thanks, Daniel > --- > drivers/gpu/drm/i915/intel_ddi.c | 7 +++++-- > drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------ > drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++-- > 3 files changed, 27 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index cacb07b..7b7f544 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; > > + ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); > if (visited[port]) > continue; > > @@ -1779,7 +1781,8 @@ 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 transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; > 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_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > index 4813374..db518ef 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 (intel_encoder->type == INTEL_OUTPUT_DSI) > + 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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>-----Original Message----- >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Friday, October 2, 2015 6:05 PM >To: Shankar, Uma >Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit >Subject: Re: [Intel-gfx] [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder >support in CRTC modeset > >On Thu, Oct 01, 2015 at 10:23:49PM +0530, Uma Shankar wrote: >> From: Shashank Sharma <shashank.sharma@intel.com> >> >> 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@intel.com> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com> > >Ok, after this patch we get stuff like this: > > 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, > true); > dev_priv->display.fdi_link_train(crtc); > } > > if (!is_dsi) > intel_ddi_enable_pipe_clock(intel_crtc); > >1. Please remove pre_pll_enable again, we don't need 2 callbacks in exactly the >same spot. Yes this might mean that you need special bxt_ versions of that in the >dsi encoder, we have that everywhere. > >2. the has_pch_encoder is already something encoder-specific (it's exclusively >used by the HSW LPT CRT encoder). Now we have another one of those for the >!is_dsi case. These special-cases should be moved into the >encoder->pre_enable callbacks, that's what they're for. > >I'm not going to block these patches are (18months is already ridiculous), but I >want this cleanup done. Uma, can you pls own this? If you can't do it yourself >please escalate to Indranil so he can find someone. > >Thanks, Daniel > Hi Daniel, I will discuss with Indranil and will get this done. Thanks for your support in getting the video mode patches merged. Regards, Uma Shankar >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++-- >> drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------ >> drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++-- >> 3 files changed, 27 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index cacb07b..7b7f544 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; >> >> + ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); >> if (visited[port]) >> continue; >> >> @@ -1779,7 +1781,8 @@ 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 transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; >> 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_opregion.c >> b/drivers/gpu/drm/i915/intel_opregion.c >> index 4813374..db518ef 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 (intel_encoder->type == INTEL_OUTPUT_DSI) >> + 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 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
On Mon, 05 Oct 2015, "Shankar, Uma" <uma.shankar@intel.com> wrote: >>-----Original Message----- >>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter >>Sent: Friday, October 2, 2015 6:05 PM >>To: Shankar, Uma >>Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit >>Subject: Re: [Intel-gfx] [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder >>support in CRTC modeset >> >>On Thu, Oct 01, 2015 at 10:23:49PM +0530, Uma Shankar wrote: >>> From: Shashank Sharma <shashank.sharma@intel.com> >>> >>> 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@intel.com> >>> Signed-off-by: Uma Shankar <uma.shankar@intel.com> >> >>Ok, after this patch we get stuff like this: >> >> 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, >> true); >> dev_priv->display.fdi_link_train(crtc); >> } >> >> if (!is_dsi) >> intel_ddi_enable_pipe_clock(intel_crtc); >> >>1. Please remove pre_pll_enable again, we don't need 2 callbacks in exactly the >>same spot. Yes this might mean that you need special bxt_ versions of that in the >>dsi encoder, we have that everywhere. >> >>2. the has_pch_encoder is already something encoder-specific (it's exclusively >>used by the HSW LPT CRT encoder). Now we have another one of those for the >>!is_dsi case. These special-cases should be moved into the >>encoder->pre_enable callbacks, that's what they're for. >> >>I'm not going to block these patches are (18months is already ridiculous), but I >>want this cleanup done. Uma, can you pls own this? If you can't do it yourself >>please escalate to Indranil so he can find someone. >> >>Thanks, Daniel >> > > Hi Daniel, > I will discuss with Indranil and will get this done. > > Thanks for your support in getting the video mode patches merged. I'll do the cleanups, don't worry about it. BR, Jani. > > Regards, > Uma Shankar > >>> --- >>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++-- >>> drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------ >>> drivers/gpu/drm/i915/intel_opregion.c | 9 +++++++-- >>> 3 files changed, 27 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >>> b/drivers/gpu/drm/i915/intel_ddi.c >>> index cacb07b..7b7f544 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; >>> >>> + ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); >>> if (visited[port]) >>> continue; >>> >>> @@ -1779,7 +1781,8 @@ 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 transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; >>> 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_opregion.c >>> b/drivers/gpu/drm/i915/intel_opregion.c >>> index 4813374..db518ef 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 (intel_encoder->type == INTEL_OUTPUT_DSI) >>> + 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 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>-- >>Daniel Vetter >>Software Engineer, Intel Corporation >>http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index cacb07b..7b7f544 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; + ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); if (visited[port]) continue; @@ -1779,7 +1781,8 @@ 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 transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder; 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_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 4813374..db518ef 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 (intel_encoder->type == INTEL_OUTPUT_DSI) + 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: