[Pull v2] Glamor - fixed build failure, fixed coding style problem.

Tue Oct 18 09:08:32 PDT 2011

On Oct 18, 2011, at 2:23 AM, Zhigang Gong wrote:

>> -----Original Message-----
>> From: Jeremy Huddleston [mailto:jeremyhu at apple.com]
>> Sent: Tuesday, October 18, 2011 3:56 PM
>> To: Zhigang Gong
>> Cc: 'Keith Packard'; xorg-devel at lists.x.org
>> Subject: Re: [Pull v2] Glamor - fixed build failure, fixed coding style
>> problem.
>> 
>> FWIW, there are a couple conflicts with my pending PULL request, but
>> they're trivial to address:
>> 
>> CONFLICT (content): Merge conflict in include/xorg-config.h.in CONFLICT
>> (content): Merge conflict in hw/xfree86/dixmods/Makefile.am
>> 
>> Your spacing in  hw/xfree86/dixmods/Makefile.am is bad, 
> Could you point out where is the bad space? I failed to find it out. Thanks.

There's an extra newline between each of these hunks to increase readability:
libfb_la_LDFLAGS = -avoid-version
libfb_la_LIBADD = $(top_builddir)/fb/libfb.la
libfb_la_SOURCES = $(top_builddir)/fb/fbcmap_mi.c fbmodule.c
libfb_la_CFLAGS = $(AM_CFLAGS)

> 
>> and you're
>> missing -module (which should be more obvious that it's necessary when
>> my PULL is merged).
> I found the other dix modules don't use -module. Shall we add -module for
> all
> the dix modules here?  Or just after your pull, we need to add -module for
> these dix modules including libfb, libdbe, etc.

The patch in my PULL fixes this for all existing modules.  You will just need to mirror that for yours.

> 
>> 
>> There are hardly any (maybe none?) reviewed-by tags in there, and some
>> are missing sign-off tags.  You should really send this to the list for
> review
>> before sending a pull request.
> emm, I know, so I have been asking comments from the list since about 1
> month ago.

I did a search for "glamor" on the list, and I only saw your first [PULL] request.  I did not see these patches sent for review.

> I will continue to improve the code according to the comments. Maybe I
> should not 
> use "Pull" in the topic.

That would be wise.

You should send each patch to the list individually.  I suggest you use 'git send-email' for a large set.

>> 
>> Also, what about Soren's comment:
>> """
>> It would make a lot of sense to add hardware acceleration to pixman, since
>> that way both X, cairo, and spice could benefit from it. It could be done
>> either through GL or by directly talking to the hardware.
>> """
>> 
>> So wouldn't this be better inside of pixman?
> Glamor's purpose is to provide a GL based rendering acceleration framework 
> which is dedicated for X server. Glamor don't want to provide any generic
> API
> to other libraries currently.

Perhaps a good introduction of this project to the list is in order.  As far as I understand it, Glamor is your baby, and you went and did this because it seemed like a cool project to work on.  I commend you for that, but there is a fair amount of surprise on this list and hesitation about taking such a large code-dump into our repository without any introduction or review.

Using GL to accelerate 2D drawing in the server sounds great, but why should we include this particular implementation and promise to support it for the next decade or more?  We want to avoid things landing in the server "just because", and we want to make sure that code is maintainable.  Having this support land in pixman seems like the right approach to me because it simplifies the dependency chain and avoids redundancy.

So what problem does Glamor solve that adding similar GL support to pixman would not fix?

> 
> I know cairo already has GL backend for application usage, but currently
> there is
> no easy way to let the xserver to leverage cairo to do the rendering.

Right, which is why such support should be brought to the next level higher, pixman.

> As to
> the 
> pixman, IMO, it's a 2D library focus on rasterizing by using CPU only.

Yeah, but seeing as Soren was the one who brought up using GL in pixman, I am fairly confident that he would be open to the idea of extending that support to using the GPU.

> For
> the client 
> side application, cairo is a good 2D libraries to utilize GPU hardware
> acceleration.
> I'm not familiar with spice. Just googled it, and it seems that it's a
> protocol to offload
> some CPU/GPU intensive tasks to remote client. I think it's also out of
> glamor's scope.

Right, but glamor's scope is defined by you, right.  Wouldn't it be better to move this code to a higher level in the stack, so *everyone* could benefit.  As it is now, only Xephyr and Xorg benefit.  By moving it into pixman, you can remove redundant code in cairo, and *every* DDX will be able to use glamor.

> 
> Thanks for your comments.
> -- zhigang
>> 
>> 
>> On Oct 18, 2011, at 12:25 AM, Zhigang Gong wrote:
>> 
>>> Hi Keith,
>>> 
>>> I just came back from a long vacation and I checked all the comments
>>> for the glamor's pull request. All the comments came from Alan. Thanks
>> Alan.
>>> Here are a summary of those comments and current status:
>>> 
>>> 1.  Alan reported a build fail problem.
>>> Fixed and tested by Alan.
>>> 2.  Alan pointed out that the coding style is not consistent with X's
>>> and even have some conflicts with the code itself.
>>> Fixed in the latest version. Alan took a quick glance, and said it's
>>> more consistent with itself. And after that, I made some change to use
>>> camelCase naming rule according to X coding style.
>>> 
>>> Please check it out and if any other thing need to be fixed, please
>>> let me know.
>>> 
>>> Thanks.
>>> 
>>> --Zhigang
>>> 
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 

---
Jeremy Huddleston

Rebuild Sudan
 - Board of Directors
 - http://www.rebuildsudan.org

Berkeley Foundation for Opportunities in Information Technology
 - Advisory Board
 - http://www.bfoit.org