>-----Original Message-----
>From: Nikula, Jani
>Sent: Friday, September 18, 2015 7:48 PM
>To: Shankar, Uma; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>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@xxxxxxxxx> wrote:
>> From: Shashank Sharma <shashank.sharma@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
