On Fri, Mar 13, 2015 at 05:32:41PM +0100, Daniel Vetter wrote:
> On Fri, Mar 13, 2015 at 05:09:46PM +0800, Zhigang Gong wrote:
> > My only concern is about the following macros:
> >
> > > +#define LOCAL_I915_PARAM_SUBSLICE_TOTAL 33
> > > +#define LOCAL_I915_PARAM_EU_TOTAL 34
> >
> > How about to just use the definitons in the kernel header file?
> > For an example:
> >
> > #include <drm/i915_drm.h>
> >
> > #ifdef LOCAL_I915_PARAM_SUBSLICE_TOTAL
> > //Put all the code into this block.
> > #endif
> >
> > Then we can avoid put the same definitions in different files,
> > and we can avoid unecessary testing on an old kernel which doesn't
> > have this kernel interface.
> >
> > For all the other part, it LGTM.
> >
> > Reviewed-by: Zhigang Gong <zhigang.gong@xxxxxxxxxxxxxxx>
>
> Once we update the libdrm requirements in igt we tend to go around and
> replace all the now obsolete LOCAL_ defines. Imo not worth doing extra
> work until then.
>
> Patch applied, thanks.
> -Daniel
>
Patch applied? Do you want me to make the name change first? Should the
kernel part be reviewed and merged first?
-Jeff
> >
> > Thanks,
> > Zhigang Gong.
> >
> > On Thu, Mar 12, 2015 at 05:26:25PM -0700, jeff.mcgee@xxxxxxxxx wrote:
> > > From: Jeff McGee <jeff.mcgee@xxxxxxxxx>
> > >
> > > New test core_getparams consists of 2 subtests, each one testing
> > > the ability of userspace to query the correct value of a GT config
> > > attribute: subslice total or EU total. drm/i915 implementation of
> > > these queries is required for Cherryview and Gen9+ devices (non-
> > > simulated).
> > >
> > > v2: Duplicate small amount of new libdrm functionality to avoid
> > > bumping libdrm version requirement (Daniel). Convert some
> > > igt_asserts to the appropriate comparison variants. Add a
> > > test description.
> > > v3: Actually use the LOCAL GETPARAM defines. Otherwise can't build
> > > against older libdrm as intended by v2.
> > >
> > > For: VIZ-4636
> > > Signed-off-by: Jeff McGee <jeff.mcgee@xxxxxxxxx>
> > > ---
> > > tests/.gitignore | 1 +
> > > tests/Makefile.sources | 1 +
> > > tests/core_getparams.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 169 insertions(+)
> > > create mode 100644 tests/core_getparams.c
> > >
> > > diff --git a/tests/.gitignore b/tests/.gitignore
> > > index 426cc67..c742308 100644
> > > --- a/tests/.gitignore
> > > +++ b/tests/.gitignore
> > > @@ -1,6 +1,7 @@
> > > # Please keep sorted alphabetically
> > > core_get_client_auth
> > > core_getclient
> > > +core_getparams
> > > core_getstats
> > > core_getversion
> > > drm_import_export
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index 51e8376..999c8f8 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -15,6 +15,7 @@ NOUVEAU_TESTS_M = \
> > >
> > > TESTS_progs_M = \
> > > core_get_client_auth \
> > > + core_getparams \
> > > drv_suspend \
> > > drv_hangman \
> > > gem_bad_reloc \
> > > diff --git a/tests/core_getparams.c b/tests/core_getparams.c
> > > new file mode 100644
> > > index 0000000..2855d06
> > > --- /dev/null
> > > +++ b/tests/core_getparams.c
> > > @@ -0,0 +1,167 @@
> > > +/*
> > > + * Copyright © 2014 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining a
> > > + * copy of this software and associated documentation files (the "Software"),
> > > + * to deal in the Software without restriction, including without limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the next
> > > + * paragraph) shall be included in all copies or substantial portions of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + *
> > > + * Authors:
> > > + * Jeff McGee <jeff.mcgee@xxxxxxxxx>
> > > + *
> > > + */
> > > +
> > > +#include <unistd.h>
> > > +#include <errno.h>
> > > +#include <xf86drm.h>
> > > +#include <i915_drm.h>
> > > +#include "drmtest.h"
> > > +#include "intel_chipset.h"
> > > +#include "intel_bufmgr.h"
> > > +
> > > +IGT_TEST_DESCRIPTION("Tests the export of parameters via DRM_IOCTL_I915_GETPARAM\n");
> > > +
> > > +int drm_fd;
> > > +int devid;
> > > +
> > > +static void
> > > +init(void)
> > > +{
> > > + drm_fd = drm_open_any();
> > > + devid = intel_get_drm_devid(drm_fd);
> > > +}
> > > +
> > > +static void
> > > +deinit(void)
> > > +{
> > > + close(drm_fd);
> > > +}
> > > +
> > > +#define LOCAL_I915_PARAM_SUBSLICE_TOTAL 33
> > > +#define LOCAL_I915_PARAM_EU_TOTAL 34
> > > +
> > > +static int
> > > +getparam(int param, int *value)
> > > +{
> > > + drm_i915_getparam_t gp;
> > > + int ret;
> > > +
> > > + memset(&gp, 0, sizeof(gp));
> > > + gp.value = value;
> > > + gp.param = param;
> > > + ret = drmIoctl(drm_fd, DRM_IOCTL_I915_GETPARAM, &gp);
> > > + if (ret)
> > > + return -errno;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void
> > > +subslice_total(void)
> > > +{
> > > + unsigned int subslice_total = 0;
> > > + int ret;
> > > +
> > > + ret = getparam(LOCAL_I915_PARAM_SUBSLICE_TOTAL, (int*)&subslice_total);
> > > +
> > > + if (ret) {
> > > + /*
> > > + * These devices are not required to implement the
> > > + * interface. If they do not, -ENODEV must be returned.
> > > + */
> > > + if ((intel_gen(devid) < 8) ||
> > > + IS_BROADWELL(devid) ||
> > > + igt_run_in_simulation()) {
> > > + igt_assert_eq(ret, -ENODEV);
> > > + igt_info("subslice total: unknown\n");
> > > + /*
> > > + * All other devices must implement the interface, so
> > > + * fail them if we are here.
> > > + */
> > > + } else {
> > > + igt_assert_neq(ret, EINVAL); /* request not recognized? */
> > > + igt_assert_neq(ret, ENODEV); /* device not supported? */
> > > + igt_assert_eq(ret, 0); /* other error? */
> > > + }
> > > + } else {
> > > + /*
> > > + * On success, just make sure the returned count value is
> > > + * non-zero. The validity of the count value for the given
> > > + * device is not checked.
> > > + */
> > > + igt_assert_neq(subslice_total, 0);
> > > + igt_info("subslice total: %u\n", subslice_total);
> > > + }
> > > +}
> > > +
> > > +static void
> > > +eu_total(void)
> > > +{
> > > + unsigned int eu_total = 0;
> > > + int ret;
> > > +
> > > + ret = getparam(LOCAL_I915_PARAM_EU_TOTAL, (int*)&eu_total);
> > > +
> > > + if (ret) {
> > > + /*
> > > + * These devices are not required to implement the
> > > + * interface. If they do not, -ENODEV must be returned.
> > > + */
> > > + if ((intel_gen(devid) < 8) ||
> > > + IS_BROADWELL(devid) ||
> > > + igt_run_in_simulation()) {
> > > + igt_assert_eq(ret, -ENODEV);
> > > + igt_info("EU total: unknown\n");
> > > + /*
> > > + * All other devices must implement the interface, so
> > > + * fail them if we are here.
> > > + */
> > > + } else {
> > > + igt_assert_neq(ret, EINVAL); /* request not recognized? */
> > > + igt_assert_neq(ret, ENODEV); /* device not supported? */
> > > + igt_assert_eq(ret, 0); /* other error? */
> > > + }
> > > + } else {
> > > + /*
> > > + * On success, just make sure the returned count value is
> > > + * non-zero. The validity of the count value for the given
> > > + * device is not checked.
> > > + */
> > > + igt_assert_neq(eu_total, 0);
> > > + igt_info("EU total: %u\n", eu_total);
> > > + }
> > > +}
> > > +
> > > +static void
> > > +exit_handler(int sig)
> > > +{
> > > + deinit();
> > > +}
> > > +
> > > +igt_main
> > > +{
> > > + igt_fixture {
> > > + igt_install_exit_handler(exit_handler);
> > > + init();
> > > + }
> > > +
> > > + igt_subtest("subslice-total")
> > > + subslice_total();
> > > +
> > > + igt_subtest("eu-total")
> > > + eu_total();
> > > +}
> > > --
> > > 2.3.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Beignet mailing list
> Beignet@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/beignet
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
