| Message ID | 1351711396-6373-1-git-send-email-damien.lespiau@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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);