diff mbox

[03/12] Don't use GetScratchPixmapHeader for shadow pixmaps

Message ID 1406243908-1123-4-git-send-email-keithp@keithp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Keith Packard July 24, 2014, 11:18 p.m. UTC
GetScratchPixmapHeader should only be used for local memory pixmaps,
as used by PutImage and friends. That's because when you free the
scratch pixmap header, it doesn't actually free the pixmap; instead,
it gets stuffed in pScreen->pScratchPixmap and any private data stored
on it will be left hanging around forever.

In the case of glamor, that private data includes all of the GL
state. Using that scratch pixmap later results in glamor getting
mightily confused as the pixmap and underlying objects do not match.

Avoid this by allocating pixmap headers explicitly for this purpose.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 src/uxa/intel_display.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

Comments

Eric Anholt July 31, 2014, 1:33 a.m. UTC | #1
Keith Packard <keithp@keithp.com> writes:

> GetScratchPixmapHeader should only be used for local memory pixmaps,
> as used by PutImage and friends. That's because when you free the
> scratch pixmap header, it doesn't actually free the pixmap; instead,
> it gets stuffed in pScreen->pScratchPixmap and any private data stored
> on it will be left hanging around forever.
>
> In the case of glamor, that private data includes all of the GL
> state. Using that scratch pixmap later results in glamor getting
> mightily confused as the pixmap and underlying objects do not match.
>
> Avoid this by allocating pixmap headers explicitly for this purpose.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  src/uxa/intel_display.c | 44 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
> index 0b83140..bcaafaa 100644
> --- a/src/uxa/intel_display.c
> +++ b/src/uxa/intel_display.c
> @@ -545,13 +545,31 @@ intel_crtc_shadow_allocate(xf86CrtcPtr crtc, int width, int height)
>  		return NULL;
>  	}
>  
> -	drm_intel_bo_disable_reuse(intel_crtc->rotate_bo);
> -
>  	intel_crtc->rotate_pitch = rotate_pitch;
>  	return intel_crtc->rotate_bo;
>  }

See comment below...

> @@ -602,8 +620,8 @@ intel_crtc_shadow_destroy(xf86CrtcPtr crtc, PixmapPtr rotate_pixmap, void *data)
>  	struct intel_mode *mode = intel_crtc->mode;
>  
>  	if (rotate_pixmap) {
> -		intel_set_pixmap_bo(rotate_pixmap, NULL);
> -		FreeScratchPixmapHeader(rotate_pixmap);
> +                intel_set_pixmap_bo(rotate_pixmap, NULL);
> +                rotate_pixmap->drawable.pScreen->DestroyPixmap(rotate_pixmap);
>  	}
>  
>  	if (data) {
> @@ -1415,7 +1433,6 @@ intel_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height)
>  	int	    i, old_width, old_height, old_pitch;
>  	int pitch;
>  	uint32_t tiling;
> -	ScreenPtr screen;
>  
>  	if (scrn->virtualX == width && scrn->virtualY == height)
>  		return TRUE;
> @@ -1430,8 +1447,7 @@ intel_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height)
>  	old_front = intel->front_buffer;
>  
>  	if (intel->back_pixmap) {
> -		screen = intel->back_pixmap->drawable.pScreen;
> -		screen->DestroyPixmap(intel->back_pixmap);
> +		scrn->pScreen->DestroyPixmap(intel->back_pixmap);
>  		intel->back_pixmap = NULL;
>  	}
>  

Grumble, unrelated noise in the patch.

> @@ -1454,7 +1470,6 @@ intel_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height)
>  	if (ret)
>  		goto fail;
>  
> -	drm_intel_bo_disable_reuse(intel->front_buffer);
>  	intel->front_pitch = pitch;
>  	intel->front_tiling = tiling;
>  

This change appears to be unrelated, and possibly harmful (if X has
dropped the last ref to the BO, but it's still the scanout buffer, a new
allocation would now reuse the BO and scribble on scanout until the next
modeset happens).

> @@ -2204,6 +2219,7 @@ Bool intel_crtc_on(xf86CrtcPtr crtc)
>  	return ret;
>  }
>  
> +
>  static PixmapPtr
>  intel_create_pixmap_for_bo(ScreenPtr pScreen, dri_bo *bo,
>  			   int width, int height,

Unrelated whitespace.
Keith Packard July 31, 2014, 5:49 a.m. UTC | #2
Eric Anholt <eric@anholt.net> writes:

> This change appears to be unrelated, and possibly harmful (if X has
> dropped the last ref to the BO, but it's still the scanout buffer, a new
> allocation would now reuse the BO and scribble on scanout until the next
> modeset happens).

Yeah, it's unrelated. intel_allocate_framebuffer calls disable_reuse, so
there's no need to call it from these two other places. I'll split that
change out into a separate patch with separate comment.

> Unrelated whitespace.

There are a bunch of whitespace fixups; should I pull those into a
separate patch or just leave them scattered in the first patch to change
a file?
Eric Anholt Aug. 4, 2014, 4:58 p.m. UTC | #3
Keith Packard <keithp@keithp.com> writes:

> Eric Anholt <eric@anholt.net> writes:
>
>> This change appears to be unrelated, and possibly harmful (if X has
>> dropped the last ref to the BO, but it's still the scanout buffer, a new
>> allocation would now reuse the BO and scribble on scanout until the next
>> modeset happens).
>
> Yeah, it's unrelated. intel_allocate_framebuffer calls disable_reuse, so
> there's no need to call it from these two other places. I'll split that
> change out into a separate patch with separate comment.
>
>> Unrelated whitespace.
>
> There are a bunch of whitespace fixups; should I pull those into a
> separate patch or just leave them scattered in the first patch to change
> a file?

One patch at the front is fine with me.
diff mbox

Patch

diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
index 0b83140..bcaafaa 100644
--- a/src/uxa/intel_display.c
+++ b/src/uxa/intel_display.c
@@ -545,13 +545,31 @@  intel_crtc_shadow_allocate(xf86CrtcPtr crtc, int width, int height)
 		return NULL;
 	}
 
-	drm_intel_bo_disable_reuse(intel_crtc->rotate_bo);
-
 	intel_crtc->rotate_pitch = rotate_pitch;
 	return intel_crtc->rotate_bo;
 }
 
 static PixmapPtr
+intel_create_pixmap_header(ScreenPtr pScreen, int width, int height, int depth,
+                           int bitsPerPixel, int devKind, void *pPixData)
+{
+        PixmapPtr pixmap;
+
+        /* width and height of 0 means don't allocate any pixmap data */
+        pixmap = (*pScreen->CreatePixmap) (pScreen, 0, 0, depth, 0);
+
+        if (pixmap) {
+                if ((*pScreen->ModifyPixmapHeader) (pixmap, width, height, depth,
+                                                    bitsPerPixel, devKind, pPixData))
+                {
+                        return pixmap;
+                }
+                (*pScreen->DestroyPixmap) (pixmap);
+        }
+        return NullPixmap;
+}
+
+static PixmapPtr
 intel_crtc_shadow_create(xf86CrtcPtr crtc, void *data, int width, int height)
 {
 	ScrnInfoPtr scrn = crtc->scrn;
@@ -573,12 +591,12 @@  intel_crtc_shadow_create(xf86CrtcPtr crtc, void *data, int width, int height)
 		return NULL;
 	}
 
-	rotate_pixmap = GetScratchPixmapHeader(scrn->pScreen,
-					       width, height,
-					       scrn->depth,
-					       scrn->bitsPerPixel,
-					       intel_crtc->rotate_pitch,
-					       NULL);
+	rotate_pixmap = intel_create_pixmap_header(scrn->pScreen,
+                                                   width, height,
+                                                   scrn->depth,
+                                                   scrn->bitsPerPixel,
+                                                   intel_crtc->rotate_pitch,
+                                                   NULL);
 
 	if (rotate_pixmap == NULL) {
 		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
@@ -602,8 +620,8 @@  intel_crtc_shadow_destroy(xf86CrtcPtr crtc, PixmapPtr rotate_pixmap, void *data)
 	struct intel_mode *mode = intel_crtc->mode;
 
 	if (rotate_pixmap) {
-		intel_set_pixmap_bo(rotate_pixmap, NULL);
-		FreeScratchPixmapHeader(rotate_pixmap);
+                intel_set_pixmap_bo(rotate_pixmap, NULL);
+                rotate_pixmap->drawable.pScreen->DestroyPixmap(rotate_pixmap);
 	}
 
 	if (data) {
@@ -1415,7 +1433,6 @@  intel_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height)
 	int	    i, old_width, old_height, old_pitch;
 	int pitch;
 	uint32_t tiling;
-	ScreenPtr screen;
 
 	if (scrn->virtualX == width && scrn->virtualY == height)
 		return TRUE;
@@ -1430,8 +1447,7 @@  intel_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height)
 	old_front = intel->front_buffer;
 
 	if (intel->back_pixmap) {
-		screen = intel->back_pixmap->drawable.pScreen;
-		screen->DestroyPixmap(intel->back_pixmap);
+		scrn->pScreen->DestroyPixmap(intel->back_pixmap);
 		intel->back_pixmap = NULL;
 	}
 
@@ -1454,7 +1470,6 @@  intel_xf86crtc_resize(ScrnInfoPtr scrn, int width, int height)
 	if (ret)
 		goto fail;
 
-	drm_intel_bo_disable_reuse(intel->front_buffer);
 	intel->front_pitch = pitch;
 	intel->front_tiling = tiling;
 
@@ -2204,6 +2219,7 @@  Bool intel_crtc_on(xf86CrtcPtr crtc)
 	return ret;
 }
 
+
 static PixmapPtr
 intel_create_pixmap_for_bo(ScreenPtr pScreen, dri_bo *bo,
 			   int width, int height,