>-----Original Message----- >From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter >Sent: Wednesday, September 23, 2015 6:42 PM >To: Nikula, Jani >Cc: Daniel Vetter; Shankar, Uma; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Kumar, Shobhit >Subject: Re: [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@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 >-- 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
