Re: [PATCH 10/12] Add glamor back into the driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux