[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.vetter at 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 at 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 <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.
>
>I like this idea, since it also aligns more with the rework we need to do For that
>one we probably need to add a ddi_port to the crtc config and make sure only
>ddi encoder related code (for hdmi/dp and vga w/ fdi) use it.
>
>Seems like the simpler solution with less interim detour overall.
>-Daniel
>--

The suggestion looks good. Will remove the WARN_ON spilled all over and put the warning in intel_ddi_get_encoder_port
instead, if it's a NON DSI encoder.

Thanks Jani for the suggestion.

Regards,
Uma Shankar