Re: [PATCH] Enable dithering on intel VCH DVO chips on 18bpp panels

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Sat, Mar 28, 2015 at 11:07:09AM +0100, Thomas Richter wrote:
> Hi folks,
> 
> this is a patch against drm-intel-nightly that enables an apparently 
> undocumented feature of the intel VCH DVO chips. Bit 4 of the VR01 
> register controls an automatic dithering for 18bpp outputs which greatly 
> improves the visual image quality for 24bpp display buffers.
> 
> The bios of my IBM R31 automatically enables this feature, it is 
> disabled again by the ivchr driver. The following patch checks whether 
> the display is connected with a 18bpp connection and if so, re-enables 
> dithering.
> 
> This feature has been tested to work fine on 16bpp images and with the 
> scaler on and off. There are no ill side effects.
> 
> Please let me know whether this patch is acceptable.
> 
> Thanks and greetings,
> 
> 	Thomas
> 
> 
> Signed-off-by: Thomas Richter <thor@math.tu-berlin.de>
> ---
>   drivers/gpu/drm/i915/dvo_ivch.c |   21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/dvo_ivch.c 
> b/drivers/gpu/drm/i915/dvo_ivch.c
> index 0f2587f..89b08a8 100644
> --- a/drivers/gpu/drm/i915/dvo_ivch.c
> +++ b/drivers/gpu/drm/i915/dvo_ivch.c
> @@ -23,6 +23,9 @@
>    * Authors:
>    *    Eric Anholt <eric@anholt.net>
>    *
> + * Minor modifications (Dithering enable):
> + *    Thomas Richter <thor@math.tu-berlin.de>
> + *
>    */
> 
>   #include "dvo.h"
> @@ -59,6 +62,8 @@
>   # define VR01_DVO_BYPASS_ENABLE                (1 << 1)
>   /** Enables the DVO clock */
>   # define VR01_DVO_ENABLE               (1 << 0)
> +/** Enable dithering for 18bpp panels. Not documented. */
> +# define VR01_DITHER_ENABLE             (1 << 4)
> 
>   /*
>    * LCD Interface Format
> @@ -74,6 +79,8 @@
>   # define VR10_INTERFACE_2X18           (2 << 2)
>   /** Enables 2x24-bit LVDS output */
>   # define VR10_INTERFACE_2X24           (3 << 2)
> +/** Mask that defines the depth of the pipeline */
> +# define VR10_INTERFACE_DEPTH_MASK      (3 << 2)
> 
>   /*
>    * VR20 LCD Horizontal Display Size
> @@ -342,9 +349,15 @@ static void ivch_mode_set(struct intel_dvo_device *dvo,
>                            struct drm_display_mode *adjusted_mode)
>   {
>          uint16_t vr40 = 0;
> -       uint16_t vr01;
> +       uint16_t vr01 = 0;
> +       uint16_t vr10;
> +
> +       ivch_read(dvo, VR10, &vr10);
> +       /* Enable dithering for 18 bpp pipelines */
> +       vr10 &= VR10_INTERFACE_DEPTH_MASK;
> +       if (vr10 == VR10_INTERFACE_2X18 || vr10 == VR10_INTERFACE_1X18)
> +               vr01 = VR01_DITHER_ENABLE;
> 
> -       vr01 = 0;
>          vr40 = (VR40_STALL_ENABLE | VR40_VERTICAL_INTERP_ENABLE |
>                  VR40_HORIZONTAL_INTERP_ENABLE);
> 
> @@ -353,7 +366,7 @@ static void ivch_mode_set(struct intel_dvo_device *dvo,
>                  uint16_t x_ratio, y_ratio;
> 
>                  vr01 |= VR01_PANEL_FIT_ENABLE;
> -               vr40 |= VR40_CLOCK_GATING_ENABLE;
> +               vr40 |= VR40_CLOCK_GATING_ENABLE | 
> VR40_ENHANCED_PANEL_FITTING;

You're also enabling this other bit. The spec isn't very clear on most
things, but I might assume this enables the higher order upscale filter.
I don't see any programmable parameters for the filter in spec, so it
might be safe to just enable it always. But yeah, not really sure about
that. OTOH I don't expect this DVO chip to be in use in many other
machines, so chances of breaking some other machine seem rather low.

However it would seems better if this was a separate patch, or of it's
really just a part of getting the dither bit to work then it would be
good to at least mention that fact in the commit message, or maybe
add a comment into the code explaining the relationship of the two bits
(assuming there is one).

>                  x_ratio = (((mode->hdisplay - 1) << 16) /
>                             (adjusted_mode->hdisplay - 1)) >> 2;
>                  y_ratio = (((mode->vdisplay - 1) << 16) /
> @@ -380,6 +393,8 @@ static void ivch_dump_regs(struct intel_dvo_device *dvo)
>          DRM_DEBUG_KMS("VR00: 0x%04x\n", val);
>          ivch_read(dvo, VR01, &val);
>          DRM_DEBUG_KMS("VR01: 0x%04x\n", val);
> +       ivch_read(dvo, VR10, &val);
> +       DRM_DEBUG_KMS("VR10: 0x%04x\n", val);
>          ivch_read(dvo, VR30, &val);
>          DRM_DEBUG_KMS("VR30: 0x%04x\n", val);
>          ivch_read(dvo, VR40, &val);
> --
> 1.7.10.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx