| Message ID | 1406243908-1123-4-git-send-email-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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?
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 --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,
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(-)