From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] Revert "drm/i915: Mask reserved bits in display/sprite address registers" Date: Fri, 24 Jan 2014 13:04:56 +0200 Message-ID: <20140124110456.GZ9454@intel.com> References: <1390556411-9008-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 2B5AAFB5A4 for ; Fri, 24 Jan 2014 03:05:04 -0800 (PST) Content-Disposition: inline In-Reply-To: <1390556411-9008-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: Dave Airlie , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, Jan 24, 2014 at 10:40:11AM +0100, Daniel Vetter wrote: > This reverts commit 446f254566ea8911c9e19c7bc8a162fc0e53cf31. > = > I've left the masking in the pageflip code since that seems to be some > useful piece of preemptive robustness. What masking where? > Iirc I've merged this patch under the assumption that the BIOS leaves > some random gunk in the lower bits and gets unhappy if we trample on > them. We have quite a few case like this, so this made sense. > = > Now I've just learned that there's actual hardware features bits in > the low 12 bits, and the kernel needs to preserve them to allow a > userspace blob to do its job. Given Dave Airlie's clear stance on > userspace blob drivers I've quickly chatted with him and he doesn't > seem too happy. So let's revert this. > = > If there are indeed bits that we must preserve in this range then we > can ressurrect this patch, but with proper documentation for those > bits supplied. And we probably also need to think a bit about > interactions with our driver. > = > Cc: Armin Reese > Cc: Jesse Barnes > Cc: Dave Airlie > Signed-off-by: Daniel Vetter I always disliked this code. Reviewed-by: Ville Syrj=E4l=E4 > --- > drivers/gpu/drm/i915/i915_reg.h | 2 -- > drivers/gpu/drm/i915/intel_display.c | 8 ++++---- > drivers/gpu/drm/i915/intel_sprite.c | 18 +++++++++--------- > 3 files changed, 13 insertions(+), 15 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_= reg.h > index ba0550222269..a48b7cad6f11 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3578,8 +3578,6 @@ > #define DISP_BASEADDR_MASK (0xfffff000) > #define I915_LO_DISPBASE(val) (val & ~DISP_BASEADDR_MASK) > #define I915_HI_DISPBASE(val) (val & DISP_BASEADDR_MASK) > -#define I915_MODIFY_DISPBASE(reg, gfx_addr) \ > - (I915_WRITE((reg), (gfx_addr) | I915_LO_DISPBASE(I915_READ(reg)))) > = > /* VBIOS flags */ > #define SWF00 (dev_priv->info->display_mmio_offset + 0x71410) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 98371eeac77c..40a9338ad54f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2114,8 +2114,8 @@ static int i9xx_update_plane(struct drm_crtc *crtc,= struct drm_framebuffer *fb, > fb->pitches[0]); > I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]); > if (INTEL_INFO(dev)->gen >=3D 4) { > - I915_MODIFY_DISPBASE(DSPSURF(plane), > - i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset); > + I915_WRITE(DSPSURF(plane), > + i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset); > I915_WRITE(DSPTILEOFF(plane), (y << 16) | x); > I915_WRITE(DSPLINOFF(plane), linear_offset); > } else > @@ -2205,8 +2205,8 @@ static int ironlake_update_plane(struct drm_crtc *c= rtc, > i915_gem_obj_ggtt_offset(obj), linear_offset, x, y, > fb->pitches[0]); > I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]); > - I915_MODIFY_DISPBASE(DSPSURF(plane), > - i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset); > + I915_WRITE(DSPSURF(plane), > + i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset); > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { > I915_WRITE(DSPOFFSET(plane), (y << 16) | x); > } else { > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/i= ntel_sprite.c > index fe4de89c374c..716a3c9c0751 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -141,8 +141,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm= _crtc *crtc, > = > I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w); > I915_WRITE(SPCNTR(pipe, plane), sprctl); > - I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj)= + > - sprsurf_offset); > + I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) + > + sprsurf_offset); > POSTING_READ(SPSURF(pipe, plane)); > } > = > @@ -158,7 +158,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct dr= m_crtc *crtc) > I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) & > ~SP_ENABLE); > /* Activate double buffered register update */ > - I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0); > + I915_WRITE(SPSURF(pipe, plane), 0); > POSTING_READ(SPSURF(pipe, plane)); > = > intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false); > @@ -315,8 +315,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_= crtc *crtc, > if (intel_plane->can_scale) > I915_WRITE(SPRSCALE(pipe), sprscale); > I915_WRITE(SPRCTL(pipe), sprctl); > - I915_MODIFY_DISPBASE(SPRSURF(pipe), > - i915_gem_obj_ggtt_offset(obj) + sprsurf_offset); > + I915_WRITE(SPRSURF(pipe), > + i915_gem_obj_ggtt_offset(obj) + sprsurf_offset); > POSTING_READ(SPRSURF(pipe)); > } > = > @@ -333,7 +333,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm= _crtc *crtc) > if (intel_plane->can_scale) > I915_WRITE(SPRSCALE(pipe), 0); > /* Activate double buffered register update */ > - I915_MODIFY_DISPBASE(SPRSURF(pipe), 0); > + I915_WRITE(SPRSURF(pipe), 0); > POSTING_READ(SPRSURF(pipe)); > = > /* > @@ -489,8 +489,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_= crtc *crtc, > I915_WRITE(DVSSIZE(pipe), (crtc_h << 16) | crtc_w); > I915_WRITE(DVSSCALE(pipe), dvsscale); > I915_WRITE(DVSCNTR(pipe), dvscntr); > - I915_MODIFY_DISPBASE(DVSSURF(pipe), > - i915_gem_obj_ggtt_offset(obj) + dvssurf_offset); > + I915_WRITE(DVSSURF(pipe), > + i915_gem_obj_ggtt_offset(obj) + dvssurf_offset); > POSTING_READ(DVSSURF(pipe)); > } > = > @@ -506,7 +506,7 @@ ilk_disable_plane(struct drm_plane *plane, struct drm= _crtc *crtc) > /* Disable the scaler */ > I915_WRITE(DVSSCALE(pipe), 0); > /* Flush double buffered register updates */ > - I915_MODIFY_DISPBASE(DVSSURF(pipe), 0); > + I915_WRITE(DVSSURF(pipe), 0); > POSTING_READ(DVSSURF(pipe)); > = > /* > -- = > 1.8.5.2 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC