[Intel-gfx] [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset
Jani Nikula
jani.nikula at linux.intel.com
Tue Sep 29 00:29:00 PDT 2015
On Mon, 28 Sep 2015, "Shankar, Uma" <uma.shankar at intel.com> wrote:
>>-----Original Message-----
>>From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>>Sent: Monday, September 28, 2015 6:58 PM
>>To: Shankar, Uma; intel-gfx at lists.freedesktop.org
>>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 at intel.com> wrote:
>>> From: Shashank Sharma <shashank.sharma at 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.
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>>> ---
>>> 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@intel.com
[2] http://mid.gmane.org/1443511466-8017-2-git-send-email-jani.nikula@intel.com
>
>>>
>>> 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
More information about the Intel-gfx
mailing list