Re: [PATCH 6/9] drm/i915: Add eDP support for Valleyview

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 27, 2012 at 07:08:41PM +0530, Vijay Purushothaman wrote:
> On 9/26/2012 8:19 PM, Daniel Vetter wrote:
> >On Wed, Sep 26, 2012 at 04:31:46PM +0200, Daniel Vetter wrote:
> >>On Wed, Sep 26, 2012 at 07:07:35PM +0530, Vijay Purushothaman wrote:
> >>>Eventhough Valleyview display block is derived from Cantiga, VLV
> >>>supports eDP. So, added eDP checks in i9xx_crtc_mode_set path.
> >>>
> >>>v2: use different DPIO_DIVISOR values for VGA, DP and eDP
> >>>v3: fix DPIO value calculation to use same values for all display
> >>>	interfaces
> >>>
> >>>Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
> >>>Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> >>>Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> >>>---
> >>>  drivers/gpu/drm/i915/intel_display.c |    6 ++++++
> >>>  drivers/gpu/drm/i915/intel_dp.c      |   13 ++++++++-----
> >>>  2 files changed, 14 insertions(+), 5 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>index a8a81d1..aee6151 100644
> >>>--- a/drivers/gpu/drm/i915/intel_display.c
> >>>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>>@@ -4405,6 +4405,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> >>>  		}
> >>>  	}
> >>>
> >>>+	if (IS_VALLEYVIEW(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP)) {
> >>>+		pipeconf |= PIPECONF_BPP_6 |
> >>>+			PIPECONF_ENABLE |
> >>>+			I965_PIPECONF_ACTIVE;
> >>>+	}
> >>
> >>No.
> >>
> >>Jani Nikula and me just figured out that we have a giant mess with 6bpc
> >>dithering on DP outputs, but unconditionally enabling 6bpc on vlv eDP only
> >>papers over issues.
> >
> >Forgotten to put Jani on cc.
> >-Daniel
> 
> Thanks for the catch. I've removed this unconditional enabling of
> 6bpc for VLV eDP. For long term i believe, eDP handling in
> i9xx_crtc_mode_set should be changed along the lines of
> ironlake_crtc_mode_set for cleaner code. For now, this should
> unblock others with VLV enabling.

My Big Plan (tm) for handling bpc, bandwidth, dotclocks and clocks in
general (e.g. pll sharing) is to move all this stuff out of the ->mode_set
callbacks and into a new preparation step at the beginning of the modeset
sequence (i.e. before we start touching the hw). Essentially I want to
extend the current ->mode_adjust callbacks and move all these calculations
in there. Aims for this are
- should allow us to move a lot of the encoder specific code that
  currently sits in the xxx_crtc_mode_set functions into encoder callbacks
- allows us to fail a modeset that won't work (e.g. due to pll sharing
  limits or fdi link bw limits) _before_ we touch the hw
- prepares for the global modeset stuff, where we want to make clever
  decision about shared resources (e.g. enable 6bpc dithering if there are
  not enough fdi lanes availbale in 3 pipe configs).

So the ironlake bpc code is a bit cleaner, but imho still gets it wrong.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch