Keith Packard <keithp@xxxxxxxxxx> writes:
> This adds glamor support back into the driver, but instad of going
> through UXA, this uses it directly instead.
This is hard to read with the conditionalizing all of the UXA code in
the same commit as adding the glamor code. Then there are a bunch of
unrelated whitespace changes, or flattening of a bunch of nested
conditionals.
The only substantive problem I see is intel_glamor_set_pixmap_bo().
> +static void intel_check_accel_option(ScrnInfoPtr scrn)
> +{
> + intel_screen_private *intel = intel_get_screen_private(scrn);
> + enum { NONE, SNA, UXA, GLAMOR } accel_method = DEFAULT_ACCEL_METHOD;
> + const char *s;
> +
> + s = xf86GetOptValString(intel->Options, OPTION_ACCEL_METHOD);
> + if (s != NULL) {
> +#if USE_GLAMOR
> + if (strcasecmp(s, "glamor") == 0)
> + accel_method = GLAMOR;
> + else
> +#endif
> +#if USE_UXA
> + if (strcasecmp(s, "uxa") == 0)
> + accel_method = UXA;
> + else
> +#endif
> + accel_method = DEFAULT_ACCEL_METHOD;
> + }
> + switch (accel_method) {
> + default:
> +#if USE_GLAMOR
> + case GLAMOR:
> + intel->accel = ACCEL_GLAMOR;
> + break;
> +#endif
> +#if USE_UXA
> + case UXA:
> + intel->accel = ACCEL_UXA;
> + break;
> +#endif
> + }
The extra enum and temporary variable introduced here seems pretty
pointless (even if that pattern had happened before).
> diff --git a/src/uxa/intel_glamor.c b/src/uxa/intel_glamor.c
> index 21636d1..e2bc24c 100644
> --- a/src/uxa/intel_glamor.c
> +++ b/src/uxa/intel_glamor.c
> +Bool
> +intel_glamor_set_pixmap_bo(PixmapPtr pixmap, drm_intel_bo *bo)
> +{
> + if (bo == NULL || glamor_egl_create_textured_pixmap(pixmap,
> + bo->handle,
> + intel_pixmap_pitch(pixmap)))
> + {
> + intel_glamor_reference_pixmap_bo(pixmap, bo);
> + return TRUE;
> + }
> + return FALSE;
> +}
I don't think this will work -- glamor_egl uses a different fd from
intel->bufmgr, so you're attaching some unrelated BO, if you're even
that lucky.
> Bool
> intel_glamor_init(ScreenPtr screen)
> {
> ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
> - intel_screen_private *intel = intel_get_screen_private(scrn);
>
> - if ((intel->uxa_flags & UXA_GLAMOR_EGL_INITIALIZED) == 0)
> - goto fail;
> + if (!dixPrivateKeyRegistered(&intel_glamor_pixmap_key))
> + if (!dixRegisterPrivateKey(&intel_glamor_pixmap_key, PRIVATE_PIXMAP, sizeof (struct intel_glamor_pixmap)))
> + return FALSE;
No need to check for initiailization -- double-calling it is safe (as
long as the size is consistent).
> void
> @@ -258,18 +240,52 @@ intel_glamor_flush(intel_screen_private * intel)
> ScreenPtr screen;
>
> screen = xf86ScrnToScreen(intel->scrn);
> - if (intel->uxa_flags & UXA_USE_GLAMOR)
> - glamor_block_handler(screen);
> + glamor_block_handler(screen);
> }
glamor_block_handler() is pretty awfully named. We should do something
about that.
Attachment:
pgpu3K9MSOGGR.pgp
Description: PGP signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
