From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 06/76] drm/i915: simplify dvo dpms interface Date: Wed, 29 Aug 2012 10:09:18 -0700 Message-ID: <20120829100918.6a9db578@jbarnes-x220> References: <1343328581-2324-1-git-send-email-daniel.vetter@ffwll.ch> <1343328581-2324-7-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 oproxy6-pub.bluehost.com (oproxy6-pub.bluehost.com [67.222.54.6]) by gabe.freedesktop.org (Postfix) with SMTP id 07BC69F36F for ; Wed, 29 Aug 2012 10:09:20 -0700 (PDT) In-Reply-To: <1343328581-2324-7-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 Thu, 26 Jul 2012 20:48:31 +0200 Daniel Vetter wrote: > All dvo drivers only support 2 dpms states, and our dvo driver > even switches of the dvo port for anything else than DPMS_ON. Hence > ditch this complexity and simply use bool enable. > > While reading through this code I've noticed that the mode_set > function of ch7017 is a bit peculiar - it disable the lvds again, even > though the crtc helper code should have done that ... This might be to > work around an issue at driver load, we pretty much ignore the hw > state when taking over. > > v2: Also do the conversion for the new ns2501 driver. > > Signed-Off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/dvo.h | 9 ++++----- > drivers/gpu/drm/i915/dvo_ch7017.c | 8 ++++---- > drivers/gpu/drm/i915/dvo_ch7xxx.c | 4 ++-- > drivers/gpu/drm/i915/dvo_ivch.c | 8 ++++---- > drivers/gpu/drm/i915/dvo_ns2501.c | 14 ++++++-------- > drivers/gpu/drm/i915/dvo_sil164.c | 4 ++-- > drivers/gpu/drm/i915/dvo_tfp410.c | 4 ++-- > drivers/gpu/drm/i915/intel_dvo.c | 4 ++-- > 8 files changed, 26 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/dvo.h b/drivers/gpu/drm/i915/dvo.h > index 0c8ac4d..0fa839e 100644 > --- a/drivers/gpu/drm/i915/dvo.h > +++ b/drivers/gpu/drm/i915/dvo.h > @@ -58,13 +58,12 @@ struct intel_dvo_dev_ops { > void (*create_resources)(struct intel_dvo_device *dvo); > > /* > - * Turn on/off output or set intermediate power levels if > available. > + * Turn on/off output. > * > - * Unsupported intermediate modes drop to the lower power > setting. > - * If the mode is DPMSModeOff, the output must be disabled, > - * as the DPLL may be disabled afterwards. > + * Because none of our dvo drivers support an intermediate > power levels, > + * we don't expose this in the interfac. > */ > - void (*dpms)(struct intel_dvo_device *dvo, int mode); > + void (*dpms)(struct intel_dvo_device *dvo, bool enable); > > /* > * Callback for testing a video mode for a given output. > diff --git a/drivers/gpu/drm/i915/dvo_ch7017.c > b/drivers/gpu/drm/i915/dvo_ch7017.c index 1ca799a..71e7650 100644 > --- a/drivers/gpu/drm/i915/dvo_ch7017.c > +++ b/drivers/gpu/drm/i915/dvo_ch7017.c > @@ -163,7 +163,7 @@ struct ch7017_priv { > }; > > static void ch7017_dump_regs(struct intel_dvo_device *dvo); > -static void ch7017_dpms(struct intel_dvo_device *dvo, int mode); > +static void ch7017_dpms(struct intel_dvo_device *dvo, bool enable); > > static bool ch7017_read(struct intel_dvo_device *dvo, u8 addr, u8 > *val) { > @@ -309,7 +309,7 @@ static void ch7017_mode_set(struct > intel_dvo_device *dvo, lvds_power_down = > CH7017_LVDS_POWER_DOWN_DEFAULT_RESERVED | (mode->hdisplay & 0x0700) > >> 8; > - ch7017_dpms(dvo, DRM_MODE_DPMS_OFF); > + ch7017_dpms(dvo, false); > ch7017_write(dvo, CH7017_HORIZONTAL_ACTIVE_PIXEL_INPUT, > horizontal_active_pixel_input); > ch7017_write(dvo, CH7017_HORIZONTAL_ACTIVE_PIXEL_OUTPUT, > @@ -331,7 +331,7 @@ static void ch7017_mode_set(struct > intel_dvo_device *dvo, } > > /* set the CH7017 power state */ > -static void ch7017_dpms(struct intel_dvo_device *dvo, int mode) > +static void ch7017_dpms(struct intel_dvo_device *dvo, bool enable) > { > uint8_t val; > > @@ -345,7 +345,7 @@ static void ch7017_dpms(struct intel_dvo_device > *dvo, int mode) CH7017_DAC3_POWER_DOWN | > CH7017_TV_POWER_DOWN_EN); > > - if (mode == DRM_MODE_DPMS_ON) { > + if (enable) { > /* Turn on the LVDS */ > ch7017_write(dvo, CH7017_LVDS_POWER_DOWN, > val & ~CH7017_LVDS_POWER_DOWN_EN); > diff --git a/drivers/gpu/drm/i915/dvo_ch7xxx.c > b/drivers/gpu/drm/i915/dvo_ch7xxx.c index 4a03660..c1dea5b 100644 > --- a/drivers/gpu/drm/i915/dvo_ch7xxx.c > +++ b/drivers/gpu/drm/i915/dvo_ch7xxx.c > @@ -289,9 +289,9 @@ static void ch7xxx_mode_set(struct > intel_dvo_device *dvo, } > > /* set the CH7xxx power state */ > -static void ch7xxx_dpms(struct intel_dvo_device *dvo, int mode) > +static void ch7xxx_dpms(struct intel_dvo_device *dvo, bool enable) > { > - if (mode == DRM_MODE_DPMS_ON) > + if (enable) > ch7xxx_writeb(dvo, CH7xxx_PM, CH7xxx_PM_DVIL | > CH7xxx_PM_DVIP); else > ch7xxx_writeb(dvo, CH7xxx_PM, CH7xxx_PM_FPD); > diff --git a/drivers/gpu/drm/i915/dvo_ivch.c > b/drivers/gpu/drm/i915/dvo_ivch.c index 04f2893..fa8ff6b 100644 > --- a/drivers/gpu/drm/i915/dvo_ivch.c > +++ b/drivers/gpu/drm/i915/dvo_ivch.c > @@ -288,7 +288,7 @@ static enum drm_mode_status > ivch_mode_valid(struct intel_dvo_device *dvo, } > > /** Sets the power state of the panel connected to the ivch */ > -static void ivch_dpms(struct intel_dvo_device *dvo, int mode) > +static void ivch_dpms(struct intel_dvo_device *dvo, bool enable) > { > int i; > uint16_t vr01, vr30, backlight; > @@ -297,13 +297,13 @@ static void ivch_dpms(struct intel_dvo_device > *dvo, int mode) if (!ivch_read(dvo, VR01, &vr01)) > return; > > - if (mode == DRM_MODE_DPMS_ON) > + if (enable) > backlight = 1; > else > backlight = 0; > ivch_write(dvo, VR80, backlight); > > - if (mode == DRM_MODE_DPMS_ON) > + if (enable) > vr01 |= VR01_LCD_ENABLE | VR01_DVO_ENABLE; > else > vr01 &= ~(VR01_LCD_ENABLE | VR01_DVO_ENABLE); > @@ -315,7 +315,7 @@ static void ivch_dpms(struct intel_dvo_device > *dvo, int mode) if (!ivch_read(dvo, VR30, &vr30)) > break; > > - if (((vr30 & VR30_PANEL_ON) != 0) == (mode == > DRM_MODE_DPMS_ON)) > + if (((vr30 & VR30_PANEL_ON) != 0) == enable) > break; > udelay(1000); > } > diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c > b/drivers/gpu/drm/i915/dvo_ns2501.c index 6bd383d..c4d9f2f 100644 > --- a/drivers/gpu/drm/i915/dvo_ns2501.c > +++ b/drivers/gpu/drm/i915/dvo_ns2501.c > @@ -493,19 +493,19 @@ static void ns2501_mode_set(struct > intel_dvo_device *dvo, } > > /* set the NS2501 power state */ > -static void ns2501_dpms(struct intel_dvo_device *dvo, int mode) > +static void ns2501_dpms(struct intel_dvo_device *dvo, bool enable) > { > bool ok; > bool restore = false; > struct ns2501_priv *ns = (struct ns2501_priv > *)(dvo->dev_priv); unsigned char ch; > > - DRM_DEBUG_KMS("%s: Trying set the dpms of the DVO to %d\n", > - __FUNCTION__, mode); > + DRM_DEBUG_KMS("%s: Trying set the dpms of the DVO to %i\n", > + __FUNCTION__, enable); > > ch = ns->reg_8_shadow; > > - if (mode == DRM_MODE_DPMS_ON) > + if (enable) > ch |= NS2501_8_PD; > else > ch &= ~NS2501_8_PD; > @@ -519,12 +519,10 @@ static void ns2501_dpms(struct intel_dvo_device > *dvo, int mode) ok &= ns2501_writeb(dvo, NS2501_REG8, ch); > ok &= > ns2501_writeb(dvo, 0x34, > - (mode == > - DRM_MODE_DPMS_ON) ? > (0x03) : (0x00)); > + enable ? 0x03 : 0x00); > ok &= > ns2501_writeb(dvo, 0x35, > - (mode == > - DRM_MODE_DPMS_ON) ? > (0xff) : (0x00)); > + enable ? 0xff : 0x00); > if (!ok) { > if (restore) > restore_dvo(dvo); > diff --git a/drivers/gpu/drm/i915/dvo_sil164.c > b/drivers/gpu/drm/i915/dvo_sil164.c index a0b13a6..cc24c1c 100644 > --- a/drivers/gpu/drm/i915/dvo_sil164.c > +++ b/drivers/gpu/drm/i915/dvo_sil164.c > @@ -208,7 +208,7 @@ static void sil164_mode_set(struct > intel_dvo_device *dvo, } > > /* set the SIL164 power state */ > -static void sil164_dpms(struct intel_dvo_device *dvo, int mode) > +static void sil164_dpms(struct intel_dvo_device *dvo, bool enable) > { > int ret; > unsigned char ch; > @@ -217,7 +217,7 @@ static void sil164_dpms(struct intel_dvo_device > *dvo, int mode) if (ret == false) > return; > > - if (mode == DRM_MODE_DPMS_ON) > + if (enable) > ch |= SIL164_8_PD; > else > ch &= ~SIL164_8_PD; > diff --git a/drivers/gpu/drm/i915/dvo_tfp410.c > b/drivers/gpu/drm/i915/dvo_tfp410.c index aa2cd3e..097b3e8 100644 > --- a/drivers/gpu/drm/i915/dvo_tfp410.c > +++ b/drivers/gpu/drm/i915/dvo_tfp410.c > @@ -234,14 +234,14 @@ static void tfp410_mode_set(struct > intel_dvo_device *dvo, } > > /* set the tfp410 power state */ > -static void tfp410_dpms(struct intel_dvo_device *dvo, int mode) > +static void tfp410_dpms(struct intel_dvo_device *dvo, bool enable) > { > uint8_t ctl1; > > if (!tfp410_readb(dvo, TFP410_CTL_1, &ctl1)) > return; > > - if (mode == DRM_MODE_DPMS_ON) > + if (enable) > ctl1 |= TFP410_CTL_1_PD; > else > ctl1 &= ~TFP410_CTL_1_PD; > diff --git a/drivers/gpu/drm/i915/intel_dvo.c > b/drivers/gpu/drm/i915/intel_dvo.c index 03dfdff..227551f 100644 > --- a/drivers/gpu/drm/i915/intel_dvo.c > +++ b/drivers/gpu/drm/i915/intel_dvo.c > @@ -115,9 +115,9 @@ static void intel_dvo_dpms(struct drm_encoder > *encoder, int mode) if (mode == DRM_MODE_DPMS_ON) { > I915_WRITE(dvo_reg, temp | DVO_ENABLE); > I915_READ(dvo_reg); > - intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, mode); > + intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true); > } else { > - intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, mode); > + intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, false); > I915_WRITE(dvo_reg, temp & ~DVO_ENABLE); > I915_READ(dvo_reg); > } Looks reasonable. The comment about lvds disable probably doesn't need to be in the commit message, but it would be good to test that theory (need to find a tester though; maybe krh has his old system lying around somewhere). Jesse