>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >Sent: Tuesday, September 29, 2015 12:59 PM >To: Shankar, Uma; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank >Subject: RE: [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in >CRTC modeset > >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. > Thanks Jani. I will rebase and re-submit. Personally I feel this will be best approach as it will avoid the call itself. However, only flip side is protection at multiple places in code for DSI. Regards, Uma Shankar >[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
