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

Wed Oct 19 01:02:01 PDT 2011

> -----Original Message-----
> From: xorg-devel-bounces+zhigang.gong=linux.intel.com at lists.x.org
> [mailto:xorg-devel-bounces+zhigang.gong=linux.intel.com at lists.x.org] On
> Behalf Of Jeremy Huddleston
> Sent: Wednesday, October 19, 2011 12:09 AM
> To: Zhigang Gong
> Cc: xorg-devel at lists.x.org
> Subject: Re: [Pull v2] Glamor - fixed build failure, fixed coding style
> problem.
> 
> 
> 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)
Agreed, and sent a patch to you for review. Thanks.

> 
> 
> >
> >> 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.
I really don't know how to do that efficiently.  You know, we have too many
patches
which may flood the list if I send them one by one. I did discuss this issue
with Eric and
Keith. And the suggestion is to submit a pull request including all the
patches here and
ask for comments and reviews from community. 
> 
> >>
> >> 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.
> 
To be honest, Glamor is initiated by Eric, I have been following his work
for several 
months. I gave a short introduction at the first pull request in the xorg
list, although 
that may be not very detail. 

> 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?
IMO, Glamor's major contribution is to introduce a new hardware independent
ddx
driver for X server and can still utilize the GPU's acceleration power
through 
OpenGL interface. I agree that there are several duplicate functions in
glamor
and cairo's GL backend. I guess that at the time when Eric started to
develop 
glamor there is no complete cairo's GL backend. 
Eric, correct me if I'm wrong.

I also agree with you that for those duplicate rendering functions we may
reduce
them to one single place, and reuse them among all the other libraries. One
possible thing is that let glamor to call cairo directly to simply glamor's
implementation.

> 
> 
> >
> > 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.
> 
Just as I said above. Your thought do make sense. Currently glamor
implements
some GL based rendering functions. And in the long term, we may merge those 
functions into cairo or pixman, and let glamor call them directly. Just as
the relation
between libfb(which belongs to X server) and pixman.

To achieve that goal, I think we still need more time. And now I want to
open glamor
at this point, and move to that direction step by step. And eventually,
there will be
a thinner glamor module. 

-- zhigang
> 
> >
> > 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
> 
> _______________________________________________
> 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