Re: [Intel-gfx] [PATCH 03/12] Don't use GetScratchPixmapHeader for shadow pixmaps

2014-08-04 Thread Eric Anholt
Keith Packard  writes:

> Eric Anholt  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.


pgpytHLVSAeTJ.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 03/12] Don't use GetScratchPixmapHeader for shadow pixmaps

2014-08-03 Thread Eric Anholt
Keith Packard  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 
> ---
>  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.


pgpBxJNfF4O_i.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 03/12] Don't use GetScratchPixmapHeader for shadow pixmaps

2014-07-30 Thread Keith Packard
Eric Anholt  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.pack...@intel.com


pgpius4ilGE8T.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx