On Thu, Mar 25, 2010 at 7:58 PM, Jakob Bornecrantz <wallbra...@gmail.com> wrote: > On Thu, Mar 25, 2010 at 3:12 PM, George Sapountzis > <gsapount...@gmail.com> wrote: >> I pushed this to master, swrastg_dri was added to the autoconf build >> system only. >> >> I also did a merge with gallium-resources for st/dri at >> ~gsap7/gallium-resources-merge-drisw, it basically reverts the old >> conversion, merges and redoes the conversion because there were a lot >> of conflicts, hope this is ok. > > Hi George, > > Let me first of say that despite my negative tone in the following > text I do really like what you have done, thanks for figuring it out > and doing the work. > > I really don't like the way you reuse the dri state tracker file to > produce two different o file form the same c file. Magic defines on > the build line that make the c file take different paths make the code > harder to read and there is a big chance that some paths just bit rot. > However to does seems to be very limited. And I also realize that the > is no way around it due to the fact that you have to work against two > different dri_util.h files and as such two different dri interfaces. >
I agree it's not pretty either. It's basically like having different CFLAGS for multiple .la's in automake. It's "limited" to init_screen, flush_front/swap_buffers/alloc_buffers which is exactly where the DRI versions are pretty different. > I have attached 4 patches that I like to run by you before pushing. > 0001 is rather trivial and is just a code cleanup that adds dri2 > prefix for functions that are in the dri2.c file. 0002 just removes > the drisw.h from the drm build and dri[1|2].h from the sw build, just > to be extra sure we don't use the wrong functions in the wrong place > (you get a build time warning plus a link error, instead of just a > link error). > These look good. > 0003 moves drisw into dri/sw and the drm dri files into dri/drm, yet > again for clarity. > > 0004 moves the common files into a common directory. > I think that it may be better to just consolidate dri_screen.c / dri_context.c / dri_drawable.c / dri_extension.c in a single file named dri.c or dri_api.c or dri_driver_api.c similar to the glx/xlib state_tracker. There is not much left in the above files after splitting out version-specific code. Then you will have: dri_api.c or dri_driver_api.c for the DRI "window system" binding dri_st_api.c and dri1.c dri2.c drisw.c for the different versions the only outlier is dri1_helper which I agree may have a better name. But then I looked at this a lot lately and your suggestion may be better for someone looking at it after some time. > One thing that could be done to reduce the amount of #ifdefs would be > to move the tables from dri_screen.c into drisw.c and dri2.c. yes, this could be done unless the suggestion above is adopted, in which case I thinks it's probably better to keep all the dri_ functions static and the _DriverAPI tables in the common file. > And > create a "virtual" function on the screen for flush_frontbuffer > alloc_texture and so on. > This vtbl already exists, it's the st_framebuffer_iface. However you implement this, I think you would end up with either an ifdef or having the same symbol in the different DRI version files, I don't know which is better. Thanks for looking at the patches, George ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Mesa3d-dev mailing list Mesa3d-dev@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
