[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 <daniel at ffwll.ch> wrote:
> On Mon, Sep 21, 2015 at 10:41:58AM +0000, Shankar, Uma wrote:
>> 
>> 
>> >-----Original Message-----
>> >From: Nikula, Jani
>> >Sent: Friday, September 18, 2015 7:48 PM
>> >To: Shankar, Uma; intel-gfx at 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.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.
>> >>
>> >> 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      |   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 DSI in this patch: intel_prepare_ddi()
(which adds a WARN for good measure anyway), and
intel_opregion_notify_encoder().

I am wondering if it would be cleaner to check for intel_encoder->type
== INTEL_OUTPUT_DSI in these two sites *instead* of doing the call, and
having a WARN_ON(type == INTEL_OUTPUT_DSI) inside ddi_get_encoder_port.

My worry beyond this patch is that the checks for PORT_INVALID will
proliferate across the driver for no good reason other than this corner
case.


BR,
Jani.



> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center