[Intel-gfx] [PATCH 10/12] Add glamor back into the driver

[Eric Anholt]

Keith Packard <keithp at keithp.com> 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140730/88476439/attachment.sig>