[PATCH] drm/i915: Only check for valid PP_{ON, OFF}_DELAYS on pre ILK hardware

[PATCH] drm/i915: Only check for valid PP_{ON, OFF}_DELAYS on pre ILK hardware

[Damien Lespiau]

From: Damien Lespiau <damien.lespiau@intel.com>

ILK+ have this register on the PCH. This check was triggering unclaimed
writes.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 0ed6baf..87e9b92 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -762,7 +762,8 @@ void intel_setup_bios(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	 /* Set the Panel Power On/Off timings if uninitialized. */
-	if ((I915_READ(PP_ON_DELAYS) == 0) && (I915_READ(PP_OFF_DELAYS) == 0)) {
+	if (!HAS_PCH_SPLIT(dev) &&
+	    I915_READ(PP_ON_DELAYS) == 0 && I915_READ(PP_OFF_DELAYS) == 0) {
 		/* Set T2 to 40ms and T5 to 200ms */
 		I915_WRITE(PP_ON_DELAYS, 0x019007d0);
 
-- 
1.7.11.7

Re: [PATCH] drm/i915: Only check for valid PP_{ON, OFF}_DELAYS on pre ILK hardware

[Paulo Zanoni]

Hi

2012/10/31 Damien Lespiau <damien.lespiau@gmail.com>:
> From: Damien Lespiau <damien.lespiau@intel.com>
>
> ILK+ have this register on the PCH. This check was triggering unclaimed
> writes.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

The patch looks correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

The only problem is: we're not doing anything here for the
HAS_PCH_SPLIT platforms. Shouldn't we be doing something? We do have
eDP code to set the PCH_PP registers, but not LVDS code for this.
Also, each encoder probably needs different values.

So my suggestion would be: apply this patch (since it fixes a problem)
and then, in the future, maybe, move this code to the encoder-specific
callbacks, and also consider the HAS_PCH_SPLIT + LVDS case.


> ---
>  drivers/gpu/drm/i915/intel_bios.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 0ed6baf..87e9b92 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -762,7 +762,8 @@ void intel_setup_bios(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>          /* Set the Panel Power On/Off timings if uninitialized. */
> -       if ((I915_READ(PP_ON_DELAYS) == 0) && (I915_READ(PP_OFF_DELAYS) == 0)) {
> +       if (!HAS_PCH_SPLIT(dev) &&
> +           I915_READ(PP_ON_DELAYS) == 0 && I915_READ(PP_OFF_DELAYS) == 0) {
>                 /* Set T2 to 40ms and T5 to 200ms */
>                 I915_WRITE(PP_ON_DELAYS, 0x019007d0);
>
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Only check for valid PP_{ON, OFF}_DELAYS on pre ILK hardware

[Daniel Vetter]

On Thu, Nov 1, 2012 at 4:52 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> The only problem is: we're not doing anything here for the
> HAS_PCH_SPLIT platforms. Shouldn't we be doing something? We do have
> eDP code to set the PCH_PP registers, but not LVDS code for this.
> Also, each encoder probably needs different values.
>
> So my suggestion would be: apply this patch (since it fixes a problem)
> and then, in the future, maybe, move this code to the encoder-specific
> callbacks, and also consider the HAS_PCH_SPLIT + LVDS case.

It's even worse than that: This code is also run on ums setups, so
potentially we could break some old setups here ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Only check for valid PP_{ON, OFF}_DELAYS on pre ILK hardware

[Paulo Zanoni]

Hi

2012/11/1 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Nov 1, 2012 at 4:52 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> The only problem is: we're not doing anything here for the
>> HAS_PCH_SPLIT platforms. Shouldn't we be doing something? We do have
>> eDP code to set the PCH_PP registers, but not LVDS code for this.
>> Also, each encoder probably needs different values.
>>
>> So my suggestion would be: apply this patch (since it fixes a problem)
>> and then, in the future, maybe, move this code to the encoder-specific
>> callbacks, and also consider the HAS_PCH_SPLIT + LVDS case.
>
> It's even worse than that: This code is also run on ums setups, so
> potentially we could break some old setups here ...

I certainly understand how the suggestion I made may break the UMS
setup, so I really think we can "drop" my suggestion, but I see no
reason to drop the original patch, since it just avoids writing a
register that does not exist (giving us less error messages on
Haswell). Maybe this patch was just forgotten?

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



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Only check for valid PP_{ON, OFF}_DELAYS on pre ILK hardware

[Daniel Vetter]

On Thu, Nov 15, 2012 at 12:33:59PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/11/1 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Nov 1, 2012 at 4:52 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> The only problem is: we're not doing anything here for the
> >> HAS_PCH_SPLIT platforms. Shouldn't we be doing something? We do have
> >> eDP code to set the PCH_PP registers, but not LVDS code for this.
> >> Also, each encoder probably needs different values.
> >>
> >> So my suggestion would be: apply this patch (since it fixes a problem)
> >> and then, in the future, maybe, move this code to the encoder-specific
> >> callbacks, and also consider the HAS_PCH_SPLIT + LVDS case.
> >
> > It's even worse than that: This code is also run on ums setups, so
> > potentially we could break some old setups here ...
> 
> I certainly understand how the suggestion I made may break the UMS
> setup, so I really think we can "drop" my suggestion, but I see no
> reason to drop the original patch, since it just avoids writing a
> register that does not exist (giving us less error messages on
> Haswell). Maybe this patch was just forgotten?

Indeed, patch is now merged to dinq, thanks for the prod
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch