On Wed, Sep 23, 2015 at 03:43:35PM +0300, Jani Nikula wrote:
> On Wed, 23 Sep 2015, Daniel Vetter <daniel@xxxxxxxx> 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@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).
>
> 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
--
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
