On Friday, May 20, 2016 23:59 BST, robert.foss@xxxxxxxxxxxxx wrote: > --- /dev/null > +++ b/lib/intel_drm_stubs.c Since this and the header file are stubs around intel_bufmgr, it might be better to call them intel_bufmgr_stubs.[ch]. In case "_stubs" in the name brings any confusion one could use _implementation/_internal and/or other. > --- /dev/null > +++ b/lib/intel_drm_stubs.h > @@ -0,0 +1,999 @@ > +#ifndef INTEL_DRM_STUBS_H > +#define INTEL_DRM_STUBS_H > + > +#include <xf86drm.h> > + > +#include "igt_core.h" > + Please don't. There is nothing here that requires either of these includes. As-is this makes the already convoluted header inclusion worse. > +#ifdef HAVE_INTEL > +#include <i915_drm.h> > +#include <intel_bufmgr.h> > +#include <drm.h> > + Similarly, please drop the drm headers. They are provided by libdrm itself (as opposed to libdrm_intel) and thus they should be available everywhere. > + > +#else > +#define i915_execbuffer2_set_context_id(eb2, context) igt_require(false); > +#define i915_execbuffer2_get_context_id(eb2) igt_require(false); > + Do we have (m)any test that don't any intel_bufmgr functionality and/or don't explicitly require intel drm device ? Afaict those will take precedense over the above two execbuf macros, thus one can omit the above. For the rest of the patch you seems to be inlining i915_drm.h and intel_bufmgr.h which is a very bad idea imho. As-is it's fragile and hard to maintain. If we drop the i915_drm.h changes, as suggested above, and copy/import the latter header as local_intel_bufmgr.h things will be better imho. It might be worth adding a note in the releasing document to sync the file with the latest libdrm_intel release. Regards, Emil _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
