From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915: implement high-bpc + pipeconf-dither support for g4x/vlv Date: Fri, 19 Apr 2013 11:39:46 -0700 Message-ID: <20130419113946.5929a6cc@jbarnes-desktop> References: <20130419093911.4b4e72dc@jbarnes-desktop> <1366395430-7365-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy9.bluehost.com (oproxy9.bluehost.com [69.89.24.6]) by gabe.freedesktop.org (Postfix) with SMTP id A1D4CE5C3A for ; Fri, 19 Apr 2013 11:39:30 -0700 (PDT) In-Reply-To: <1366395430-7365-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, 19 Apr 2013 20:17:10 +0200 Daniel Vetter wrote: > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8c36376..7f1ab8b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4605,22 +4605,29 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc) > pipeconf &= ~PIPECONF_DOUBLE_WIDE; > } > > - /* default to 8bpc */ > - pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN); > - if (intel_crtc->config.has_dp_encoder) { > - if (intel_crtc->config.dither) { > - pipeconf |= PIPECONF_6BPC | > - PIPECONF_DITHER_EN | > + /* only g4x and later have fancy bpc/dither controls */ > + if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) { > + pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN); > + > + /* Bspec claims that we can't use dithering for 30bpp pipes. */ > + if (intel_crtc->config.dither && intel_crtc->config.pipe_bpp != 30) > + pipeconf |= PIPECONF_DITHER_EN | > PIPECONF_DITHER_TYPE_SP; > - } > - } > > - if (IS_VALLEYVIEW(dev) && intel_pipe_has_type(&intel_crtc->base, > - INTEL_OUTPUT_EDP)) { > - if (intel_crtc->config.dither) { > - pipeconf |= PIPECONF_6BPC | > - PIPECONF_ENABLE | > - I965_PIPECONF_ACTIVE; > + pipeconf &= ~PIPECONF_BPC_MASK; > + switch (intel_crtc->config.pipe_bpp) { > + case 18: > + pipeconf |= PIPECONF_6BPC; > + break; > + case 24: > + pipeconf |= PIPECONF_8BPC; > + break; > + case 30: > + pipeconf |= PIPECONF_10BPC; > + break; > + default: > + /* Case prevented by intel_choose_pipe_bpp_dither. */ > + BUG(); Am I misreading here? It looks like we may set the 10bpc pipeconf bit, but never enable it. Should there be another G4x check somewhere? Or should the 10bpc case be dropped from the switch? > } > } > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index 58a98ff..094f3c5 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -135,7 +135,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) > /* Set the dithering flag on LVDS as needed, note that there is no > * special lvds dither control bit on pch-split platforms, dithering is > * only controlled through the PIPECONF reg. */ > - if (INTEL_INFO(dev)->gen == 4) { > + if (INTEL_INFO(dev)->gen == 4 && !IS_G4X(dev)) { > if (intel_crtc->config.dither) > temp |= LVDS_ENABLE_DITHER; > else Looks ok otherwise, though I'd still like to see LVDS dither compared with pipe dither. And what happens now if a 30bpp config gets chosen for an LVDS or eDP panel? Do we just output it anyway and get junk on the screen? -- Jesse Barnes, Intel Open Source Technology Center