On Mon, 28 Sep 2015, "Shankar, Uma" <uma.shankar@xxxxxxxxx> wrote: >>-----Original Message----- >>From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >>Sent: Monday, September 28, 2015 6:58 PM >>To: Shankar, Uma; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma >>Subject: Re: [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in >>CRTC modeset >> >>On Wed, 23 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. >>> >>> 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 | 21 ++++++++++++++++++++- >>> drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------ >>> drivers/gpu/drm/i915/intel_opregion.c | 3 ++- >>> 4 files changed, 38 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>> b/drivers/gpu/drm/i915/i915_drv.h index fd1de45..78d31c5 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -142,6 +142,7 @@ enum plane { >>> #define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites[(p)] + >>> (s) + 'A') >>> >>> enum port { >>> + PORT_INVALID = -1, >> >>My idea was that you wouldn't add this. Maybe I wasn't clear enough. >> >>> PORT_A = 0, >>> PORT_B, >>> PORT_C, >>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >>> b/drivers/gpu/drm/i915/intel_ddi.c >>> index cacb07b..8edb632 100644 >>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>> @@ -227,6 +227,10 @@ static void ddi_get_encoder_port(struct >>intel_encoder *intel_encoder, >>> } else if (type == INTEL_OUTPUT_ANALOG) { >>> *dig_port = NULL; >>> *port = PORT_E; >>> + } else if (type == INTEL_OUTPUT_DSI) { >>> + *dig_port = NULL; >>> + *port = PORT_INVALID; >>> + DRM_DEBUG_KMS("Encoder type: DSI. Returning...\n"); >> >>My idea was that you'd only call this function on DDI (i.e. non-DSI) encoders. So >>you could do a warn here. Doesn't matter what you set *port to, it's going to be >>wrong anyway, and this is only slightly better than not oopsing on the BUG >>below. >> >>> } else { >>> DRM_ERROR("Invalid DDI encoder type %d\n", type); >>> BUG(); >>> @@ -237,6 +241,15 @@ enum port intel_ddi_get_encoder_port(struct >>> intel_encoder *intel_encoder) { >>> struct intel_digital_port *dig_port; >>> enum port port; >>> + int type = intel_encoder->type; >>> + >>> + if (type == INTEL_OUTPUT_DSI) { >>> + port = PORT_INVALID; >>> + DRM_DEBUG_KMS("Encoder type: DSI. Returning...\n"); >>> + WARN_ON(1); >>> + >>> + return port; >>> + } >> >>Remove these. >> > > intel_ddi_get_encoder_port is the one getting called from multiple locations. This expects an enum to be returned. We could either set the *port in > > @@ -227,6 +227,10 @@ static void ddi_get_encoder_port(struct > intel_encoder *intel_encoder, > } else if (type == INTEL_OUTPUT_ANALOG) { > *dig_port = NULL; > *port = PORT_E; > + } else if (type == INTEL_OUTPUT_DSI) { > + *dig_port = NULL; > + *port = PORT_INVALID; > + DRM_DEBUG_KMS("Encoder type: DSI. Returning...\n"); > > And let this function return the PORT_INVALID with a WARN. Or we can initialize the port to PORT_INVALID and return that instead. Then I can remove these lines from here. > Also, If we try to avoid this function getting called from various locations, we will again end up to the original problem of spilled over DSI checks at multiple places in code. > > Please suggest which ever looks ok. I just sent two patches [1][2] to modify ddi_get_encoder_port a little. Rebase on top, and don't modify ddi_get_encoder_port() or intel_ddi_get_encoder_port() at all. Just make sure you don't call the functions for DSI. No need to add PORT_INVALID either. BR, Jani. [1] http://mid.gmane.org/1443511466-8017-1-git-send-email-jani.nikula@xxxxxxxxx [2] http://mid.gmane.org/1443511466-8017-2-git-send-email-jani.nikula@xxxxxxxxx > >>> >>> ddi_get_encoder_port(intel_encoder, &dig_port, &port); >>> >>> @@ -392,6 +405,11 @@ void intel_prepare_ddi(struct drm_device *dev) >>> >>> ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port); >>> >> >>My idea was that you'd only call this function on DDI (i.e. non-DSI) encoders. So >>you'd have to add a check for DSI here. >> >>> + if (port == PORT_INVALID) { >>> + WARN_ON(1); >> >>But this warn now makes me think we don't ever get here on with DSI. Don't >>warn for normal cases. > > Yes, you are right. This shouldn't be Warning, just a DSI protection should be fine. Will rectify this. > >>> + continue; >>> + } >>> + >>> if (visited[port]) >>> continue; >>> >>> @@ -1779,7 +1797,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..326aa6b 100644 >>> --- a/drivers/gpu/drm/i915/intel_opregion.c >>> +++ b/drivers/gpu/drm/i915/intel_opregion.c >>> @@ -335,7 +335,7 @@ int intel_opregion_notify_encoder(struct >>intel_encoder *intel_encoder, >>> return 0; >>> >>> port = intel_ddi_get_encoder_port(intel_encoder); >> >>My idea was that you'd only call this function on DDI (i.e. non-DSI) encoders. So >>you'd have to add a check for DSI here. >> > > Currently, I have implemented it as all other occurrences of this function. Even if it gets called for DSI, > it will return PORT_INVALID. That case is handled below. > > But it would be better to check for DSI and avoid the call and an unnecessary Warning. Will change this. > > Please comment if the overall approach looks OK. Will implement accordingly. > > Thanks for all the suggestions and comments. > > Regards, > Uma Shankar > >>BR, >>Jani. >> >>> - if (port == PORT_E) { >>> + if ((port == PORT_E) || (port == PORT_INVALID)) { >>> port = 0; >>> } else { >>> parm |= 1 << port; >>> @@ -356,6 +356,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 -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
