On Thu, Mar 25, 2010 at 7:00 PM, George Sapountzis <gsapount...@gmail.com> wrote: > 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. >
Yeah its not as bad as it could be, and there really isn't any way around it with regard to that drisw_util.h vs dri_util.h. > >> 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. Cool I'll push those. > >> 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. I think you can do both, the moving of the files is more for making it blatantly clear what is going on. The common files live in the common directory and the rest are in their own directory. This mirrors what is done in egl. I'm ambivalent about grouping the files together. And I don't think that moving them it a common directory stops us from doing either one. Then again dri_extension.c is quite useless, and should probably be moved into dri_screen either way. Are you strongly against the layout that I suggested? I have no real problem with doing both? > >> 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. There is only one function that is static that is used in the tables, or are you meaning that we should make them static as we move all of them into a single 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. Ah yes double vtable, hmm, well anyways its not that invasive. > > Thanks for looking at the patches, No problems, thanks for looking at mine :) Cheers Jakob. ------------------------------------------------------------------------------ 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
