[PATCH 6/7] glamor: Adapt glamor_program API to handle render acceleration

[Keith Packard]

Eric Anholt <eric at anholt.net> writes:

> Summary: Tiny nitpicks, one functional concern.
>
> Keith Packard <keithp at keithp.com> writes:
>> This extends the existing API to support options needed for render
>> accleratoin, including an additional fragment, 'combine', (which
>
> acceleration
>
>> provides a place to perform the source IN mask operation before the
>> final OP dest state) and an addtional 'defines' parameter which
>
> additional

Yeah, typos in the commit message fixed.

> This would be nice as:
>     [glamor_program_alpha_normal] = "       gl_FragColor = source * mask.a;\n",
>     [glamor_program_alpha_ca_first] = "       gl_FragColor = source.a * mask;\n",
>     [glamor_program_alpha_ca_second] = "       gl_FragColor = source * mask;\n"

Agreed.

> missing space

Fixed.

> I don't think the glamor_program_source_picture path is complete -- what
> about src->transform and src->alphaMap?

Oops. I've made it bail in that case for now. Transforms shouldn't be
terribly difficult, but we can pend that for now.

> I'd be just fine with the text path only doing solids, and glyph-masking
> a texture being something we fall back to the slow path for.  If so, and
> we need to do 1x1 repeating src (I think that's how a lot of text is
> drawn still, right?), we might want to drop fill_pos in the facet and
> just texture2D(sampler, vec2(0.5)).

Checking for transform and alphaMap was easy to add, and leaving in the
large texture support isn't a huge burden. I did add a custom 1x1
version to see if that's faster (it isn't on my machine), but it might
be on something which executes more slowly?

> This was hard to read, and I think explicit might be nicer:
>
>     prog = &program_render->progs[source_type][alpha];
>     if (!glamor_setup_one_program_render(screen, prog, source_type, alpha, prim, defines))
>         return NULL;
>     if (alpha == glamor_program_alpha_ca_first) {
>         /* Make sure we can also build the second program before deciding
>          * to use this path.
>          */
>         if (!glamor_setup_one_program_render(screen,
>                                              &program_render->progs[source_type][glamor_program_alpha_ca_second],
>                                              source_type, glamor_program_alpha_ca_second, prim,
>                                              defines)) {
>             return NULL;
>         }
>     }

Your version is massively easier to read.

Here's a patch which addresses these issues on top of the current code;
if you like what it does, I'll squash it together with the original
patch.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-glamor-Respond-to-Eric-s-review-comments-about-new-R.patch
Type: text/x-diff
Size: 12342 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150512/8db8d082/attachment.patch>
-------------- next part --------------


-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150512/8db8d082/attachment.sig>