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

From: "Shankar, Uma" <uma.shankar@intel.com>
To: "Nikula, Jani" <jani.nikula@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "Kumar, Shobhit" <shobhit.kumar@intel.com>
Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset
Date: Mon, 21 Sep 2015 10:41:58 +0000	[thread overview]
Message-ID: <E7C9878FBA1C6D42A1CA3F62AEB6945F580C959A@BGSMSX104.gar.corp.intel.com> (raw)
In-Reply-To: <87vbb7q098.fsf@intel.com>

>-----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.shankar@intel.com> wrote:
>> From: Shashank Sharma <shashank.sharma@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@intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@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.

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