[Intel-gfx] [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset
Shankar, Uma
uma.shankar at intel.com
Wed Sep 30 09:33:55 PDT 2015
>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>Sent: Tuesday, September 29, 2015 12:59 PM
>To: Shankar, Uma; intel-gfx at lists.freedesktop.org
>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 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.
>
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 at intel.com
>[2] http://mid.gmane.org/1443511466-8017-2-git-send-email-
>jani.nikula at 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