Fwd: Re: [RFC i-g-t 7/7] lib/stubs: Add stubs for intel_bufmgr.

Commit Message

Robert Foss May 25, 2016, 10:08 p.m. UTC

Forward to ML.

On Wednesday, May 25, 2016 19:18 BST, [e-mail] wrote:
> From: Robert Foss <[e-mail]>
>
> This patch provides stubs for functionality otherwise provided by intel_bufmgr.
>
> The stubbed functions all fail with a call to igt_require_f(false,"").
> Defines and enums have been copied from libdrm_intel.
>
> Due to the stubbed tests failing with an igt_require_f() call, these stubs are
> not well suited for non-tests, since tools/benchmarks/etc 'skipping'

> execution is unhelpful.
>
> Signed-off-by: Robert Foss <[e-mail]>
> ---
>  lib/Makefile.am              |   7 +
>  lib/stubs/drm/intel_bufmgr.c | 275 +++++++++++++++++++++++++++
>  lib/stubs/drm/intel_bufmgr.h | 430 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 712 insertions(+)
>  create mode 100644 lib/stubs/drm/intel_bufmgr.c
>  create mode 100644 lib/stubs/drm/intel_bufmgr.h
>
And now the final piece...

> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index d2ae98d..3e12f25 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -14,6 +14,13 @@ if HAVE_LIBDRM_VC4
>          igt_vc4.h
>  endif
>
Not sure how one can do this gradually so here's the overall result... 
Actually we can do it with a lot less churn props to the following. I 
should comment on 1/7 though as it's missing some bits.


> +if HAVE_LIBDRM_INTEL
> +else
Just drop the conditional ? The if guard within the source file is enough.

> +    libintel_tools_la_SOURCES += 	\
> +        stubs/drm/intel_bufmgr.c	\
> +        stubs/drm/intel_bufmgr.h
> +endif
> +
>  AM_CPPFLAGS = -I$(top_srcdir)
>  AM_CFLAGS = $(CWARNFLAGS) $(DRM_CFLAGS) $(PCIACCESS_CFLAGS) $(LIBUNWIND_CFLAGS) $(DEBUG_CFLAGS) \
>  	    -DIGT_SRCDIR=\""$(abs_top_srcdir)/tests"\" \
> diff --git a/lib/stubs/drm/intel_bufmgr.c b/lib/stubs/drm/intel_bufmgr.c
> new file mode 100644
> index 0000000..eaf1b3e
> --- /dev/null
> +++ b/lib/stubs/drm/intel_bufmgr.c
> @@ -0,0 +1,275 @@
> +#ifndef HAVE_LIBDRM_INTEL
> +
> +#include <drm.h>
> +#include <i915_drm.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +#include "igt_core.h"
> +#include "intel_bufmgr.h"
> +
Not sure if we need all those includes. I'm leaning that at least stdint 
and stdbool should do. Please double-check.

> +
> +drm_intel_bufmgr *drm_intel_bufmgr_gem_init(int fd, int batch_size)
> +{
> +	igt_require_f(false, "Not compiled with libdrm_intel support\n");
Ideally you want something like (you might get away without the "%s")
	igt_require_f(false, "%s", compiled_without_libdrm_intel_msg);

And declare the variable at the top

static const char *compiled_without_libdrm_intel_msg = "Not compiled 
with libdrm_intel support\n";

Otherwise you'll end up with dozens of instances of the exact same string.

> +}
> +
> +#endif//HAVE_LIBDRM_INTEL
Space around //

> diff --git a/lib/stubs/drm/intel_bufmgr.h b/lib/stubs/drm/intel_bufmgr.h
> new file mode 100644
> index 0000000..12bce60
> --- /dev/null
> +++ b/lib/stubs/drm/intel_bufmgr.h
> @@ -0,0 +1,430 @@
> +#ifndef INTEL_DRM_STUBS_H
> +#define INTEL_DRM_STUBS_H
> +
This file should be an exact copy of the one provided by libdrm_intel. 
You want to add a README here and/or update the RELEASING document to 
remind people to check/resync the file before release. Ideally as a 
follow up commit.

-Emil

Patch

--- a/configure.ac
+++ b/configure.ac
@@ -155,6 +155,9 @@  PKG_CHECK_MODULES(GLIB, glib-2.0)
  if test "x$INTEL" = xyes; then
         PKG_CHECK_MODULES(DRM_INTEL, [libdrm_intel >= 2.4.64])
         AC_DEFINE(HAVE_LIBDRM_INTEL, 1, [Have intel support])
+else
+       DRM_INTEL_CFLAGS=$(top_srcdir)/lib/stubs/drm/
+       AC_SUBST([DRM_INTEL_CFLAGS])
  fi
  AM_CONDITIONAL(HAVE_LIBDRM_INTEL, [test "x$INTEL" = xyes])