Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Jani Nikula]

On Tue, 01 Sep 2015, Uma Shankar  wrote:
> @@ -5057,13 +5060,16 @@ 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);
>   drm_crtc_vblank_on(crtc);
>  
>   for_each_encoder_on_crtc(dev, crtc, encoder) {
> + if (encoder->pre_pll_enable)
> + encoder->pre_pll_enable(encoder);
> +

We should not modify the crtc enable sequence to accommodate the needs
of one encoder type only. The hook names should be taken as describing
roughly when in the sequence they are called, not necessarily what they
must do for each encoder. If an encoder requires a different ordering or
sequence, it should handle this in what it does in its hooks - and this
may possibly need to be different on each platform.

Here, the ->pre_pll_enable call is added after the ->pre_enable call,
making the sequence of calls surprising. Also, there is no point in
calling ->pre_pll_enable in the same loop as ->enable; the encoder could
just as well do everything in ->enable. Indeed, this may be what you
should do on Broxton.

I'm willing to ignore [1] if Daniel thinks my worry is unwarranted, but
this part must be fixed.


BR,
Jani.


[1] http://mid.gmane.org/87twqlnw5k@intel.com


>   encoder->enable(encoder);
>   intel_opregion_notify_encoder(encoder, true);
>   }

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Jani Nikula]

On Wed, 23 Sep 2015, Daniel Vetter  wrote:
> On Mon, Sep 21, 2015 at 10:41:58AM +, Shankar, Uma wrote:
>> 
>> 
>> >-Original Message-
>> >From: Nikula, Jani
>> >Sent: Friday, September 18, 2015 7:48 PM
>> >To: Shankar, Uma; intel-gfx@lists.freedesktop.org
>> >Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>> >Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in
>> >CRTC modeset
>> >
>> >On Tue, 01 Sep 2015, Uma Shankar  wrote:
>> >> From: Shashank Sharma 
>> >>
>> >> 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.
>> >>
>> >> Signed-off-by: Shashank Sharma 
>> >> Signed-off-by: Uma Shankar 
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_drv.h   |1 +
>> >>  drivers/gpu/drm/i915/intel_ddi.c  |   29 
>> >> -
>> >>  drivers/gpu/drm/i915/intel_display.c  |   19 ++-
>> >>  drivers/gpu/drm/i915/intel_dp_mst.c   |1 +
>> >>  drivers/gpu/drm/i915/intel_opregion.c |3 ++-
>> >>  5 files changed, 46 insertions(+), 7 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,
>> >>   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..5d5aad2 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");
>> >
>> >Please remind me again what are the legitimate paths to get here with DSI?
>> >
>> >With all the changes and warns across the driver, I'm beginning to think we
>> >should have a version of this function that accepts DSI, and another one 
>> >that
>> >(calls the other one) and WARNS on DSI, and that should be called on all 
>> >paths
>> >that should never encounter a DSI encoder.
>> >
>> >The proliferation of WARNS all over the place is not very nice.
>> >
>> >I'm sorry, I know this is not the review I gave previously on this.
>> >
>> >BR,
>> >Jani.
>> 
>> This is a tricky piece Jani. Our code for BXT extensively uses haswell 
>> functions which was a DDI only implementation.
>> So many functions just use intel_ddi_get_encoder_port (bxt_ddi_clock_get is 
>> one such example). Currently I have added
>> WARN_ON in all of these functions, though some may not get called if DSI 
>> encoder is present.  We can remove those, 
>> but still this will be a good check to have IMO.
>> 
>> Overall, I feel even if we implement two separate functions, for the generic 
>> functions to pick the correct one, we may have
>> to have a DSI check there in those generic functions.
>
> Yeah hsw+ ddi design isn't great since the split between encoder and crtc
> isn't where the crossbar is, which means there's lots of calls from crtc
> code into DDI encoder functions. I started with that reshuffling a while
> back but Paulo shot it down a bit, but I think with bxt dsi we have a good
> reason for this.
>
> Essentially all differences between DSI, DDI (hdmi or DP) and DDI in FDI
> mode (for vga on hsw) should be hidden behind intel_encoder callbacks.
>
> But since it doesn't make much sense to hold up dsi enabling for even
> longer we should do that in parallel. And for doing that refactoring
> throwing piles of WARN_ON checks at the code imo makes sense (even if it
> doesn't look pretty).

As far as I can tell, there's two calls to {intel_}ddi_get_encoder_port
that are functionally changed for 

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Daniel Vetter]

On Wed, Sep 23, 2015 at 03:43:35PM +0300, Jani Nikula wrote:
> On Wed, 23 Sep 2015, Daniel Vetter  wrote:
> > On Mon, Sep 21, 2015 at 10:41:58AM +, Shankar, Uma wrote:
> >> 
> >> 
> >> >-Original Message-
> >> >From: Nikula, Jani
> >> >Sent: Friday, September 18, 2015 7:48 PM
> >> >To: Shankar, Uma; intel-gfx@lists.freedesktop.org
> >> >Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
> >> >Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support 
> >> >in
> >> >CRTC modeset
> >> >
> >> >On Tue, 01 Sep 2015, Uma Shankar  wrote:
> >> >> From: Shashank Sharma 
> >> >>
> >> >> 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.
> >> >>
> >> >> Signed-off-by: Shashank Sharma 
> >> >> Signed-off-by: Uma Shankar 
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_drv.h   |1 +
> >> >>  drivers/gpu/drm/i915/intel_ddi.c  |   29 
> >> >> -
> >> >>  drivers/gpu/drm/i915/intel_display.c  |   19 ++-
> >> >>  drivers/gpu/drm/i915/intel_dp_mst.c   |1 +
> >> >>  drivers/gpu/drm/i915/intel_opregion.c |3 ++-
> >> >>  5 files changed, 46 insertions(+), 7 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,
> >> >> 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..5d5aad2 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");
> >> >
> >> >Please remind me again what are the legitimate paths to get here with DSI?
> >> >
> >> >With all the changes and warns across the driver, I'm beginning to think 
> >> >we
> >> >should have a version of this function that accepts DSI, and another one 
> >> >that
> >> >(calls the other one) and WARNS on DSI, and that should be called on all 
> >> >paths
> >> >that should never encounter a DSI encoder.
> >> >
> >> >The proliferation of WARNS all over the place is not very nice.
> >> >
> >> >I'm sorry, I know this is not the review I gave previously on this.
> >> >
> >> >BR,
> >> >Jani.
> >> 
> >> This is a tricky piece Jani. Our code for BXT extensively uses haswell 
> >> functions which was a DDI only implementation.
> >> So many functions just use intel_ddi_get_encoder_port (bxt_ddi_clock_get 
> >> is one such example). Currently I have added
> >> WARN_ON in all of these functions, though some may not get called if DSI 
> >> encoder is present.  We can remove those, 
> >> but still this will be a good check to have IMO.
> >> 
> >> Overall, I feel even if we implement two separate functions, for the 
> >> generic functions to pick the correct one, we may have
> >> to have a DSI check there in those generic functions.
> >
> > Yeah hsw+ ddi design isn't great since the split between encoder and crtc
> > isn't where the crossbar is, which means there's lots of calls from crtc
> > code into DDI encoder functions. I started with that reshuffling a while
> > back but Paulo shot it down a bit, but I think with bxt dsi we have a good
> > reason for this.
> >
> > Essentially all differences between DSI, DDI (hdmi or DP) and DDI in FDI
> > mode (for vga on hsw) should be hidden behind intel_encoder 

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Shankar, Uma]

>-Original Message-
>From: Nikula, Jani
>Sent: Wednesday, September 23, 2015 6:24 PM
>To: Shankar, Uma; intel-gfx@lists.freedesktop.org
>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in
>CRTC modeset
>
>On Tue, 01 Sep 2015, Uma Shankar  wrote:
>> @@ -5057,13 +5060,16 @@ 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);
>>  drm_crtc_vblank_on(crtc);
>>
>>  for_each_encoder_on_crtc(dev, crtc, encoder) {
>> +if (encoder->pre_pll_enable)
>> +encoder->pre_pll_enable(encoder);
>> +
>
>We should not modify the crtc enable sequence to accommodate the needs of
>one encoder type only. The hook names should be taken as describing roughly
>when in the sequence they are called, not necessarily what they must do for
>each encoder. If an encoder requires a different ordering or sequence, it 
>should
>handle this in what it does in its hooks - and this may possibly need to be
>different on each platform.
>
>Here, the ->pre_pll_enable call is added after the ->pre_enable call, making 
>the
>sequence of calls surprising. Also, there is no point in calling 
>->pre_pll_enable in
>the same loop as ->enable; the encoder could just as well do everything in -
>>enable. Indeed, this may be what you should do on Broxton.
>
>I'm willing to ignore [1] if Daniel thinks my worry is unwarranted, but this 
>part
>must be fixed.
>
>
>BR,
>Jani.
>

The pre_pll_enable callback was not being used earlier for any encoder in 
haswell functions.
This is the reason we used it for DSI at place appropriate for DSI sequence. I 
will remove the
callback and put the code in encoder->enable callback itself.

Regards,
Uma Shankar

>[1] http://mid.gmane.org/87twqlnw5k@intel.com
>
>
>>  encoder->enable(encoder);
>>  intel_opregion_notify_encoder(encoder, true);
>>  }
>

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Shankar, Uma]

>-Original Message-
>From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter
>Sent: Wednesday, September 23, 2015 6:42 PM
>To: Nikula, Jani
>Cc: Daniel Vetter; Shankar, Uma; intel-gfx@lists.freedesktop.org; Kumar, 
>Shobhit
>Subject: Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder
>support in CRTC modeset
>
>On Wed, Sep 23, 2015 at 03:43:35PM +0300, Jani Nikula wrote:
>> On Wed, 23 Sep 2015, Daniel Vetter <dan...@ffwll.ch> wrote:
>> > On Mon, Sep 21, 2015 at 10:41:58AM +, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-Original Message-
>> >> >From: Nikula, Jani
>> >> >Sent: Friday, September 18, 2015 7:48 PM
>> >> >To: Shankar, Uma; intel-gfx@lists.freedesktop.org
>> >> >Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>> >> >Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder
>> >> >support in CRTC modeset
>> >> >
>> >> >On Tue, 01 Sep 2015, Uma Shankar <uma.shan...@intel.com> wrote:
>> >> >> From: Shashank Sharma <shashank.sha...@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.
>> >> >>
>> >> >> Signed-off-by: Shashank Sharma <shashank.sha...@intel.com>
>> >> >> Signed-off-by: Uma Shankar <uma.shan...@intel.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/i915/i915_drv.h   |1 +
>> >> >>  drivers/gpu/drm/i915/intel_ddi.c  |   29
>-
>> >> >>  drivers/gpu/drm/i915/intel_display.c  |   19 ++-
>> >> >>  drivers/gpu/drm/i915/intel_dp_mst.c   |1 +
>> >> >>  drivers/gpu/drm/i915/intel_opregion.c |3 ++-
>> >> >>  5 files changed, 46 insertions(+), 7 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,
>> >> >>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..5d5aad2 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");
>> >> >
>> >> >Please remind me again what ar

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Shankar, Uma]

>-Original Message-
>From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
>Shankar, Uma
>Sent: Wednesday, September 23, 2015 8:19 PM
>To: Nikula, Jani; intel-gfx@lists.freedesktop.org
>Cc: Kumar, Shobhit
>Subject: Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder
>support in CRTC modeset
>
>
>
>>-Original Message-
>>From: Nikula, Jani
>>Sent: Wednesday, September 23, 2015 6:24 PM
>>To: Shankar, Uma; intel-gfx@lists.freedesktop.org
>>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>>Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder
>>support in CRTC modeset
>>
>>On Tue, 01 Sep 2015, Uma Shankar <uma.shan...@intel.com> wrote:
>>> @@ -5057,13 +5060,16 @@ 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);
>>> drm_crtc_vblank_on(crtc);
>>>
>>> for_each_encoder_on_crtc(dev, crtc, encoder) {
>>> +   if (encoder->pre_pll_enable)
>>> +   encoder->pre_pll_enable(encoder);
>>> +
>>
>>We should not modify the crtc enable sequence to accommodate the needs
>>of one encoder type only. The hook names should be taken as describing
>>roughly when in the sequence they are called, not necessarily what they
>>must do for each encoder. If an encoder requires a different ordering
>>or sequence, it should handle this in what it does in its hooks - and
>>this may possibly need to be different on each platform.
>>
>>Here, the ->pre_pll_enable call is added after the ->pre_enable call,
>>making the sequence of calls surprising. Also, there is no point in
>>calling ->pre_pll_enable in the same loop as ->enable; the encoder
>>could just as well do everything in -
>>>enable. Indeed, this may be what you should do on Broxton.
>>
>>I'm willing to ignore [1] if Daniel thinks my worry is unwarranted, but
>>this part must be fixed.
>>
>>
>>BR,
>>Jani.
>>
>
>The pre_pll_enable callback was not being used earlier for any encoder in
>haswell functions.
>This is the reason we used it for DSI at place appropriate for DSI sequence. I 
>will
>remove the callback and put the code in encoder->enable callback itself.
>
>Regards,
>Uma Shankar

This callback should ideally be before pre_enable. I will update and resend the 
patch. 
This is the order followed for BYT/CHT as well.

Regards,
Uma Shankar

The correct order for BXT is also pre_pll_enable, 
>>[1] http://mid.gmane.org/87twqlnw5k@intel.com
>>
>>
>>> encoder->enable(encoder);
>>> intel_opregion_notify_encoder(encoder, true);
>>> }
>>
>
>___
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Daniel Vetter]

On Mon, Sep 21, 2015 at 10:41:58AM +, Shankar, Uma wrote:
> 
> 
> >-Original Message-
> >From: Nikula, Jani
> >Sent: Friday, September 18, 2015 7:48 PM
> >To: Shankar, Uma; intel-gfx@lists.freedesktop.org
> >Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
> >Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in
> >CRTC modeset
> >
> >On Tue, 01 Sep 2015, Uma Shankar  wrote:
> >> From: Shashank Sharma 
> >>
> >> 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.
> >>
> >> Signed-off-by: Shashank Sharma 
> >> Signed-off-by: Uma Shankar 
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h   |1 +
> >>  drivers/gpu/drm/i915/intel_ddi.c  |   29 -
> >>  drivers/gpu/drm/i915/intel_display.c  |   19 ++-
> >>  drivers/gpu/drm/i915/intel_dp_mst.c   |1 +
> >>  drivers/gpu/drm/i915/intel_opregion.c |3 ++-
> >>  5 files changed, 46 insertions(+), 7 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,
> >>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..5d5aad2 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");
> >
> >Please remind me again what are the legitimate paths to get here with DSI?
> >
> >With all the changes and warns across the driver, I'm beginning to think we
> >should have a version of this function that accepts DSI, and another one that
> >(calls the other one) and WARNS on DSI, and that should be called on all 
> >paths
> >that should never encounter a DSI encoder.
> >
> >The proliferation of WARNS all over the place is not very nice.
> >
> >I'm sorry, I know this is not the review I gave previously on this.
> >
> >BR,
> >Jani.
> 
> This is a tricky piece Jani. Our code for BXT extensively uses haswell 
> functions which was a DDI only implementation.
> So many functions just use intel_ddi_get_encoder_port (bxt_ddi_clock_get is 
> one such example). Currently I have added
> WARN_ON in all of these functions, though some may not get called if DSI 
> encoder is present.  We can remove those, 
> but still this will be a good check to have IMO.
> 
> Overall, I feel even if we implement two separate functions, for the generic 
> functions to pick the correct one, we may have
> to have a DSI check there in those generic functions.

Yeah hsw+ ddi design isn't great since the split between encoder and crtc
isn't where the crossbar is, which means there's lots of calls from crtc
code into DDI encoder functions. I started with that reshuffling a while
back but Paulo shot it down a bit, but I think with bxt dsi we have a good
reason for this.

Essentially all differences between DSI, DDI (hdmi or DP) and DDI in FDI
mode (for vga on hsw) should be hidden behind intel_encoder callbacks.

But since it doesn't make much sense to hold up dsi enabling for even
longer we should do that in parallel. And for doing that refactoring
throwing piles of WARN_ON checks at the code imo makes sense (even if it
doesn't look pretty).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Shankar, Uma]

>-Original Message-
>From: Nikula, Jani
>Sent: Friday, September 18, 2015 7:48 PM
>To: Shankar, Uma; intel-gfx@lists.freedesktop.org
>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in
>CRTC modeset
>
>On Tue, 01 Sep 2015, Uma Shankar  wrote:
>> From: Shashank Sharma 
>>
>> 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.
>>
>> Signed-off-by: Shashank Sharma 
>> Signed-off-by: Uma Shankar 
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   |1 +
>>  drivers/gpu/drm/i915/intel_ddi.c  |   29 -
>>  drivers/gpu/drm/i915/intel_display.c  |   19 ++-
>>  drivers/gpu/drm/i915/intel_dp_mst.c   |1 +
>>  drivers/gpu/drm/i915/intel_opregion.c |3 ++-
>>  5 files changed, 46 insertions(+), 7 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,
>>  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..5d5aad2 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");
>
>Please remind me again what are the legitimate paths to get here with DSI?
>
>With all the changes and warns across the driver, I'm beginning to think we
>should have a version of this function that accepts DSI, and another one that
>(calls the other one) and WARNS on DSI, and that should be called on all paths
>that should never encounter a DSI encoder.
>
>The proliferation of WARNS all over the place is not very nice.
>
>I'm sorry, I know this is not the review I gave previously on this.
>
>BR,
>Jani.

This is a tricky piece Jani. Our code for BXT extensively uses haswell 
functions which was a DDI only implementation.
So many functions just use intel_ddi_get_encoder_port (bxt_ddi_clock_get is one 
such example). Currently I have added
WARN_ON in all of these functions, though some may not get called if DSI 
encoder is present.  We can remove those, 
but still this will be a good check to have IMO.

Overall, I feel even if we implement two separate functions, for the generic 
functions to pick the correct one, we may have
to have a DSI check there in those generic functions.

Please suggest.

Regards,
Uma Shankar

>>  } else {
>>  DRM_ERROR("Invalid DDI encoder type %d\n", type);
>>  BUG();
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Jani Nikula]

On Tue, 01 Sep 2015, Uma Shankar  wrote:
> From: Shashank Sharma 
>
> 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.
>
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Uma Shankar 
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |1 +
>  drivers/gpu/drm/i915/intel_ddi.c  |   29 -
>  drivers/gpu/drm/i915/intel_display.c  |   19 ++-
>  drivers/gpu/drm/i915/intel_dp_mst.c   |1 +
>  drivers/gpu/drm/i915/intel_opregion.c |3 ++-
>  5 files changed, 46 insertions(+), 7 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,
>   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..5d5aad2 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");

Please remind me again what are the legitimate paths to get here with
DSI?

With all the changes and warns across the driver, I'm beginning to think
we should have a version of this function that accepts DSI, and another
one that (calls the other one) and WARNS on DSI, and that should be
called on all paths that should never encounter a DSI encoder.

The proliferation of WARNS all over the place is not very nice.

I'm sorry, I know this is not the review I gave previously on this.


BR,
Jani.


>   } else {
>   DRM_ERROR("Invalid DDI encoder type %d\n", type);
>   BUG();
> @@ -392,6 +396,11 @@ void intel_prepare_ddi(struct drm_device *dev)
>  
>   ddi_get_encoder_port(intel_encoder, _dig_port, );
>  
> + if (port == PORT_INVALID) {
> + WARN_ON(1);
> + continue;
> + }
> +
>   if (visited[port])
>   continue;
>  
> @@ -980,6 +989,8 @@ static void bxt_ddi_clock_get(struct intel_encoder 
> *encoder,
>   enum port port = intel_ddi_get_encoder_port(encoder);
>   uint32_t dpll = port;
>  
> + WARN_ON(port == PORT_INVALID);
> +
>   pipe_config->port_clock =
>   bxt_calc_pll_link(dev_priv, dpll);
>  
> @@ -1572,6 +1583,8 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc 
> *crtc)
>   int type = intel_encoder->type;
>   uint32_t temp;
>  
> + WARN_ON(port == PORT_INVALID);
> +
>   /* Enable TRANS_DDI_FUNC_CTL for the pipe to work in HDMI mode */
>   temp = TRANS_DDI_FUNC_ENABLE;
>   temp |= TRANS_DDI_SELECT_PORT(port);
> @@ -1684,6 +1697,8 @@ bool intel_ddi_connector_get_hw_state(struct 
> intel_connector *intel_connector)
>   enum intel_display_power_domain power_domain;
>   uint32_t tmp;
>  
> + WARN_ON(port == PORT_INVALID);
> +
>   power_domain = intel_display_port_power_domain(intel_encoder);
>   if (!intel_display_power_is_enabled(dev_priv, power_domain))
>   return false;
> @@ -1730,6 +1745,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder 
> *encoder,
>   u32 tmp;
>   int i;
>  
> + WARN_ON(port == PORT_INVALID);
> +
>   power_domain = intel_display_port_power_domain(encoder);
>   if (!intel_display_power_is_enabled(dev_priv, power_domain))
>   return false;
> @@ -1779,11 +1796,14 @@ 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 = _crtc->base;
> - struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> + struct