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.
BR,
Jani.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
