On Mon, 05 Oct 2015, "Shankar, Uma" <uma.shankar@xxxxxxxxx> wrote: >>-----Original Message----- >>From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter >>Sent: Friday, October 2, 2015 6:05 PM >>To: Shankar, Uma >>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Kumar, Shobhit >>Subject: Re: [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@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> >> >>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@xxxxxxxxxxxxxxxxxxxxx >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>-- >>Daniel Vetter >>Software Engineer, Intel Corporation >>http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
