| Message ID | 1366362877-15446-5-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote: > Up to now we've relied on the bios to get this right for us. Let's try > out whether our code has improved a bit, since we should dither > always when the output bpp doesn't match the plane bpp. > - gen5+ should be fine, since we only use the bios hint as an upgrade. > - gen4 changes, since here dithering is still controlled in the lvds > register. > - gen2/3 has implicit dithering depeding upon whether you use 2 or 3 > lvds pairs (which makes sense, since it only supports 8bpc pipe > outpu configurations). > - hsw doesn't support lvds. > > v2: Remove redudant dither setting. > > v3: Completly drop reliance on dev_priv->lvds_dither. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------ > drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++----- > 3 files changed, 19 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index d91bad8..2a82bd8 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5160,8 +5160,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc) > } > > static void ironlake_set_pipeconf(struct drm_crtc *crtc, > - struct drm_display_mode *adjusted_mode, > - bool dither) > + struct drm_display_mode *adjusted_mode) > { > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > @@ -5190,7 +5189,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc, > } > > val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK); > - if (dither) > + if (intel_crtc->config.dither) > val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP); > > val &= ~PIPECONF_INTERLACE_MASK; > @@ -5273,8 +5272,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc) > } > > static void haswell_set_pipeconf(struct drm_crtc *crtc, > - struct drm_display_mode *adjusted_mode, > - bool dither) > + struct drm_display_mode *adjusted_mode) > { > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > @@ -5284,7 +5282,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc, > val = I915_READ(PIPECONF(cpu_transcoder)); > > val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK); > - if (dither) > + if (intel_crtc->config.dither) > val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP); > > val &= ~PIPECONF_INTERLACE_MASK_HSW; > @@ -5645,7 +5643,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > bool is_lvds = false; > struct intel_encoder *encoder; > int ret; > - bool dither, fdi_config_ok; > + bool fdi_config_ok; > > for_each_encoder_on_crtc(dev, crtc, encoder) { > switch (encoder->type) { > @@ -5680,11 +5678,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > /* Ensure that the cursor is valid for the new mode before changing... */ > intel_crtc_update_cursor(crtc, true); > > - /* determine panel color depth */ > - dither = intel_crtc->config.dither; > - if (is_lvds && dev_priv->lvds_dither) > - dither = true; > - > DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); > drm_mode_debug_printmodeline(mode); > > @@ -5752,7 +5745,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > > fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc); > > - ironlake_set_pipeconf(crtc, adjusted_mode, dither); > + ironlake_set_pipeconf(crtc, adjusted_mode); > > /* Set up the display plane register */ > I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE); > @@ -5829,7 +5822,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > bool is_cpu_edp = false; > struct intel_encoder *encoder; > int ret; > - bool dither; > > for_each_encoder_on_crtc(dev, crtc, encoder) { > switch (encoder->type) { > @@ -5865,9 +5857,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > /* Ensure that the cursor is valid for the new mode before changing... */ > intel_crtc_update_cursor(crtc, true); > > - /* determine panel color depth */ > - dither = intel_crtc->config.dither; > - > DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); > drm_mode_debug_printmodeline(mode); > > @@ -5881,7 +5870,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > if (intel_crtc->config.has_pch_encoder) > ironlake_fdi_set_m_n(crtc); > > - haswell_set_pipeconf(crtc, adjusted_mode, dither); > + haswell_set_pipeconf(crtc, adjusted_mode); > > intel_set_pipe_csc(crtc); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c20201d..e3ca7e7 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -201,6 +201,11 @@ struct intel_crtc_config { > /* DP has a bunch of special case unfortunately, so mark the pipe > * accordingly. */ > bool has_dp_encoder; > + > + /* > + * Enable dithering, used when the selected pipe bpp doesn't match the > + * plane bpp. > + */ > bool dither; > > /* Controls for the clock computation, to override various stages. */ > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index 563f505..58a98ff 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) > * special lvds dither control bit on pch-split platforms, dithering is > * only controlled through the PIPECONF reg. */ > if (INTEL_INFO(dev)->gen == 4) { > - if (dev_priv->lvds_dither) > + if (intel_crtc->config.dither) > temp |= LVDS_ENABLE_DITHER; > else > temp &= ~LVDS_ENABLE_DITHER; > @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder, > DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n", > pipe_config->pipe_bpp, lvds_bpp); > pipe_config->pipe_bpp = lvds_bpp; > + > + /* Make sure pre-965 set dither correctly */ > + if (INTEL_INFO(dev)->gen < 4) > + pfit_control |= PANEL_8TO6_DITHER_ENABLE; I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe always 8bpc, and then we should enable dithering on the port/pfit when lvds is 6bpc. Right now I think we'll start with pipe_bpp=18 when the primary plane surface is 16bpp, and then we wouldn't enable dithering here for 6bpc lvds. > + > } > + > /* > * We have timings from the BIOS for the panel, put them in > * to the adjusted mode. The CRTC will be set up for this mode, > @@ -470,10 +476,6 @@ out: > pfit_pgm_ratios = 0; > } > > - /* Make sure pre-965 set dither correctly */ > - if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither) > - pfit_control |= PANEL_8TO6_DITHER_ENABLE; > - > if (pfit_control != lvds_encoder->pfit_control || > pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) { > lvds_encoder->pfit_control = pfit_control; > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 25, 2013 at 02:57:16PM +0300, Ville Syrjälä wrote: > On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote: > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > > index 563f505..58a98ff 100644 > > --- a/drivers/gpu/drm/i915/intel_lvds.c > > +++ b/drivers/gpu/drm/i915/intel_lvds.c > > @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) > > * special lvds dither control bit on pch-split platforms, dithering is > > * only controlled through the PIPECONF reg. */ > > if (INTEL_INFO(dev)->gen == 4) { > > - if (dev_priv->lvds_dither) > > + if (intel_crtc->config.dither) > > temp |= LVDS_ENABLE_DITHER; > > else > > temp &= ~LVDS_ENABLE_DITHER; > > @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder, > > DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n", > > pipe_config->pipe_bpp, lvds_bpp); > > pipe_config->pipe_bpp = lvds_bpp; > > + > > + /* Make sure pre-965 set dither correctly */ > > + if (INTEL_INFO(dev)->gen < 4) > > + pfit_control |= PANEL_8TO6_DITHER_ENABLE; > > I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe > always 8bpc, and then we should enable dithering on the port/pfit > when lvds is 6bpc. > > Right now I think we'll start with pipe_bpp=18 when the primary plane > surface is 16bpp, and then we wouldn't enable dithering here for 6bpc > lvds. Yeah, the patch does slightly change behaviour as we no longer blindly follow the bios wrt dithering lvds. And imo trying to dither a 16bpp plane (even if the pipe is running internally at 8bpc) is a bit pointless, since there's simply no intermediate levels to dither down to 6bpc. Otoh just using the dither flag unconditionally gives us a notch more unified code. So I've opted for that. -Daniel
On Thu, Apr 25, 2013 at 02:24:50PM +0200, Daniel Vetter wrote: > On Thu, Apr 25, 2013 at 02:57:16PM +0300, Ville Syrjälä wrote: > > On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote: > > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > > > index 563f505..58a98ff 100644 > > > --- a/drivers/gpu/drm/i915/intel_lvds.c > > > +++ b/drivers/gpu/drm/i915/intel_lvds.c > > > @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) > > > * special lvds dither control bit on pch-split platforms, dithering is > > > * only controlled through the PIPECONF reg. */ > > > if (INTEL_INFO(dev)->gen == 4) { > > > - if (dev_priv->lvds_dither) > > > + if (intel_crtc->config.dither) > > > temp |= LVDS_ENABLE_DITHER; > > > else > > > temp &= ~LVDS_ENABLE_DITHER; > > > @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder, > > > DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n", > > > pipe_config->pipe_bpp, lvds_bpp); > > > pipe_config->pipe_bpp = lvds_bpp; > > > + > > > + /* Make sure pre-965 set dither correctly */ > > > + if (INTEL_INFO(dev)->gen < 4) > > > + pfit_control |= PANEL_8TO6_DITHER_ENABLE; > > > > I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe > > always 8bpc, and then we should enable dithering on the port/pfit > > when lvds is 6bpc. > > > > Right now I think we'll start with pipe_bpp=18 when the primary plane > > surface is 16bpp, and then we wouldn't enable dithering here for 6bpc > > lvds. > > Yeah, the patch does slightly change behaviour as we no longer blindly > follow the bios wrt dithering lvds. And imo trying to dither a 16bpp plane > (even if the pipe is running internally at 8bpc) is a bit pointless, since > there's simply no intermediate levels to dither down to 6bpc. Otoh just > using the dither flag unconditionally gives us a notch more unified code. > So I've opted for that. I was just wondering what happens when we have 16bpp surface so pipe_bpp is 18, and then we have 24bit lvds which means this hunk of code will enable PANEL_8TO6_DITHER_ENABLE. Is that going to produce something that still looks sensible?
On Thu, Apr 25, 2013 at 2:42 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Apr 25, 2013 at 02:24:50PM +0200, Daniel Vetter wrote: >> On Thu, Apr 25, 2013 at 02:57:16PM +0300, Ville Syrjälä wrote: >> > On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote: >> > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c >> > > index 563f505..58a98ff 100644 >> > > --- a/drivers/gpu/drm/i915/intel_lvds.c >> > > +++ b/drivers/gpu/drm/i915/intel_lvds.c >> > > @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) >> > > * special lvds dither control bit on pch-split platforms, dithering is >> > > * only controlled through the PIPECONF reg. */ >> > > if (INTEL_INFO(dev)->gen == 4) { >> > > - if (dev_priv->lvds_dither) >> > > + if (intel_crtc->config.dither) >> > > temp |= LVDS_ENABLE_DITHER; >> > > else >> > > temp &= ~LVDS_ENABLE_DITHER; >> > > @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder, >> > > DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n", >> > > pipe_config->pipe_bpp, lvds_bpp); >> > > pipe_config->pipe_bpp = lvds_bpp; >> > > + >> > > + /* Make sure pre-965 set dither correctly */ >> > > + if (INTEL_INFO(dev)->gen < 4) >> > > + pfit_control |= PANEL_8TO6_DITHER_ENABLE; >> > >> > I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe >> > always 8bpc, and then we should enable dithering on the port/pfit >> > when lvds is 6bpc. >> > >> > Right now I think we'll start with pipe_bpp=18 when the primary plane >> > surface is 16bpp, and then we wouldn't enable dithering here for 6bpc >> > lvds. >> >> Yeah, the patch does slightly change behaviour as we no longer blindly >> follow the bios wrt dithering lvds. And imo trying to dither a 16bpp plane >> (even if the pipe is running internally at 8bpc) is a bit pointless, since >> there's simply no intermediate levels to dither down to 6bpc. Otoh just >> using the dither flag unconditionally gives us a notch more unified code. >> So I've opted for that. > > I was just wondering what happens when we have 16bpp surface so > pipe_bpp is 18, and then we have 24bit lvds which means this hunk of code > will enable PANEL_8TO6_DITHER_ENABLE. Is that going to produce something > that still looks sensible? Hm, indeed I've ignored this case a bit. I guess it would result in something ok-ish, but I'm not sure. I'll do a similar check to what I've added for g4x/vlv in another patch for 30bpp modes, and simply only enabling dithering when we think it's needed and when we think the "pipe" runs at 18bpp. I'll update the patch a bit. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d91bad8..2a82bd8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5160,8 +5160,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc) } static void ironlake_set_pipeconf(struct drm_crtc *crtc, - struct drm_display_mode *adjusted_mode, - bool dither) + struct drm_display_mode *adjusted_mode) { struct drm_i915_private *dev_priv = crtc->dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); @@ -5190,7 +5189,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc, } val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK); - if (dither) + if (intel_crtc->config.dither) val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP); val &= ~PIPECONF_INTERLACE_MASK; @@ -5273,8 +5272,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc) } static void haswell_set_pipeconf(struct drm_crtc *crtc, - struct drm_display_mode *adjusted_mode, - bool dither) + struct drm_display_mode *adjusted_mode) { struct drm_i915_private *dev_priv = crtc->dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); @@ -5284,7 +5282,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc, val = I915_READ(PIPECONF(cpu_transcoder)); val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK); - if (dither) + if (intel_crtc->config.dither) val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP); val &= ~PIPECONF_INTERLACE_MASK_HSW; @@ -5645,7 +5643,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, bool is_lvds = false; struct intel_encoder *encoder; int ret; - bool dither, fdi_config_ok; + bool fdi_config_ok; for_each_encoder_on_crtc(dev, crtc, encoder) { switch (encoder->type) { @@ -5680,11 +5678,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, /* Ensure that the cursor is valid for the new mode before changing... */ intel_crtc_update_cursor(crtc, true); - /* determine panel color depth */ - dither = intel_crtc->config.dither; - if (is_lvds && dev_priv->lvds_dither) - dither = true; - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); drm_mode_debug_printmodeline(mode); @@ -5752,7 +5745,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc); - ironlake_set_pipeconf(crtc, adjusted_mode, dither); + ironlake_set_pipeconf(crtc, adjusted_mode); /* Set up the display plane register */ I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE); @@ -5829,7 +5822,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, bool is_cpu_edp = false; struct intel_encoder *encoder; int ret; - bool dither; for_each_encoder_on_crtc(dev, crtc, encoder) { switch (encoder->type) { @@ -5865,9 +5857,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, /* Ensure that the cursor is valid for the new mode before changing... */ intel_crtc_update_cursor(crtc, true); - /* determine panel color depth */ - dither = intel_crtc->config.dither; - DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe)); drm_mode_debug_printmodeline(mode); @@ -5881,7 +5870,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, if (intel_crtc->config.has_pch_encoder) ironlake_fdi_set_m_n(crtc); - haswell_set_pipeconf(crtc, adjusted_mode, dither); + haswell_set_pipeconf(crtc, adjusted_mode); intel_set_pipe_csc(crtc); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c20201d..e3ca7e7 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -201,6 +201,11 @@ struct intel_crtc_config { /* DP has a bunch of special case unfortunately, so mark the pipe * accordingly. */ bool has_dp_encoder; + + /* + * Enable dithering, used when the selected pipe bpp doesn't match the + * plane bpp. + */ bool dither; /* Controls for the clock computation, to override various stages. */ diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 563f505..58a98ff 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) * special lvds dither control bit on pch-split platforms, dithering is * only controlled through the PIPECONF reg. */ if (INTEL_INFO(dev)->gen == 4) { - if (dev_priv->lvds_dither) + if (intel_crtc->config.dither) temp |= LVDS_ENABLE_DITHER; else temp &= ~LVDS_ENABLE_DITHER; @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder, DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n", pipe_config->pipe_bpp, lvds_bpp); pipe_config->pipe_bpp = lvds_bpp; + + /* Make sure pre-965 set dither correctly */ + if (INTEL_INFO(dev)->gen < 4) + pfit_control |= PANEL_8TO6_DITHER_ENABLE; + } + /* * We have timings from the BIOS for the panel, put them in * to the adjusted mode. The CRTC will be set up for this mode, @@ -470,10 +476,6 @@ out: pfit_pgm_ratios = 0; } - /* Make sure pre-965 set dither correctly */ - if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither) - pfit_control |= PANEL_8TO6_DITHER_ENABLE; - if (pfit_control != lvds_encoder->pfit_control || pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) { lvds_encoder->pfit_control = pfit_control;
Up to now we've relied on the bios to get this right for us. Let's try out whether our code has improved a bit, since we should dither always when the output bpp doesn't match the plane bpp. - gen5+ should be fine, since we only use the bios hint as an upgrade. - gen4 changes, since here dithering is still controlled in the lvds register. - gen2/3 has implicit dithering depeding upon whether you use 2 or 3 lvds pairs (which makes sense, since it only supports 8bpc pipe outpu configurations). - hsw doesn't support lvds. v2: Remove redudant dither setting. v3: Completly drop reliance on dev_priv->lvds_dither. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------ drivers/gpu/drm/i915/intel_drv.h | 5 +++++ drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++----- 3 files changed, 19 insertions(+), 23 deletions(-)