[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-xorg-devel
Subject:    Re: [PATCH 10/12] Add glamor back into the driver
From:       Keith Packard <keithp () keithp ! com>
Date:       2014-07-31 6:23:05
Message-ID: 861tt2dp2u.fsf () hiro ! keithp ! com
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


Eric Anholt <eric@anholt.net> writes:

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

I'm not sure how to make the patch easier to review; I guess I could
make UXA conditional first? That would be 'crazy' in that the driver
would fail to ever work if you didn't ask for UXA, but might make the
patch easier to read?

> The only substantive problem I see is intel_glamor_set_pixmap_bo().

> The extra enum and temporary variable introduced here seems pretty
> pointless (even if that pattern had happened before).

Agreed. The problem is that 'DEFAULT_ACCEL_METHOD' is defined as
'GLAMOR', 'UXA' or 'NONE' by configure.ac. This seemed like the cleanest
solution in some ways. I also liked having the accel_type enum *not*
define acceleration types which weren't compiled into the driver; that
caught a few missing #ifdefs

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

This API uses the same FD as the intel bufmgr.

From intel_glamor.c:

        if (!glamor_egl_init(scrn, intel->drmSubFD)) {

From glamor_egl.c:

        Bool
        glamor_egl_init(ScrnInfoPtr scrn, int fd)
        ...

	    glamor_egl->fd = fd;
        ...                
	    glamor_egl->has_gem = glamor_egl_check_has_gem(fd);

        ...
        Bool
	glamor_egl_create_textured_pixmap(PixmapPtr pixmap, int handle, int stride)
	...
            if (glamor_egl->has_gem) {
                if (!glamor_get_flink_name(glamor_egl->fd, handle, &name)) {

So, you pass in a GEM handle for the intel driver bufmgr and glamor
does the flink to get a global name.

We should be using an FD passing mechanism here instead, to avoid
creating more global names...

> No need to check for initiailization -- double-calling it is safe (as
> long as the size is consistent).

Thanks. Will fix.

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

Suggestions welcome :-)

Thanks for the careful review.

-- 
keith.packard@intel.com

[Attachment #5 (application/pgp-signature)]

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[prev in list] [next in list] [prev in thread] [next in thread]