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@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.
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
