On Tue, Dec 13, 2016 at 04:16:41PM +0100, David Herrmann wrote:
> Hey Chris
>
> On Mon, Dec 12, 2016 at 12:53 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> > For testing, we want a reproducible PRNG, a plain linear congruent
> > generator is suitable for our very limited selftests.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/Kconfig | 5 +++++
> > drivers/gpu/drm/Makefile | 1 +
> > drivers/gpu/drm/lib/drm_rand.c | 51 ++++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/lib/drm_rand.h | 9 ++++++++
> > 4 files changed, 66 insertions(+)
> > create mode 100644 drivers/gpu/drm/lib/drm_rand.c
> > create mode 100644 drivers/gpu/drm/lib/drm_rand.h
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index fd341ab69c46..04d1d0a32c5c 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -48,10 +48,15 @@ config DRM_DEBUG_MM
> >
> > If in doubt, say "N".
> >
> > +config DRM_LIB_RAND
> > + bool
> > + default n
> > +
> > config DRM_DEBUG_MM_SELFTEST
> > tristate "kselftests for DRM range manager (struct drm_mm)"
> > depends on DRM
> > depends on DEBUG_KERNEL
> > + select DRM_LIB_RAND
> > default n
> > help
> > This option provides a kernel module that can be used to test
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index c8aed3688b20..363eb1a23151 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -18,6 +18,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
> > drm_plane.o drm_color_mgmt.o drm_print.o \
> > drm_dumb_buffers.o drm_mode_config.o
> >
> > +drm-$(CONFIG_DRM_LIB_RAND) += lib/drm_rand.o
> > obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/test-drm_mm.o
> >
> > drm-$(CONFIG_COMPAT) += drm_ioc32.o
> > diff --git a/drivers/gpu/drm/lib/drm_rand.c b/drivers/gpu/drm/lib/drm_rand.c
> > new file mode 100644
> > index 000000000000..f203c47bb525
> > --- /dev/null
> > +++ b/drivers/gpu/drm/lib/drm_rand.c
> > @@ -0,0 +1,51 @@
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#include "drm_rand.h"
> > +
> > +u32 drm_lcg_random(u32 *state)
> > +{
> > + u32 s = *state;
> > +
> > +#define rol(x, k) (((x) << (k)) | ((x) >> (32 - (k))))
> > + s = (s ^ rol(s, 5) ^ rol(s, 24)) + 0x37798849;
> > +#undef rol
>
> #include <linux/bitops.h>
>
> static inline __u32 rol32(__u32 word, unsigned int shift);
>
> Allows you to get rid of "rol()" and the temporary "u32 s;".
Ta.
>
> > +
> > + *state = s;
> > + return s;
> > +}
> > +EXPORT_SYMBOL(drm_lcg_random);
> > +
> > +int *drm_random_reorder(int *order, int count, u32 *state)
> > +{
> > + int n;
> > +
> > + for (n = count-1; n > 1; n--) {
> > + int r = drm_lcg_random(state) % (n + 1);
> > + if (r != n) {
>
> Why not drop that condition? No harm in doing the self-swap, is there?
> Makes the code shorter.
It makes more sense when it was calling a function to handle the swap.
>
> > + int tmp = order[n];
> > + order[n] = order[r];
> > + order[r] = tmp;
>
> swap() in <linux/kernel.h>
>
> > + }
> > + }
>
> Is there a specific reason to do it that way? How about:
>
> for (i = 0; i < count; ++i)
> swap(order[i], order[drm_lcg_random(state) % count]);
>
> Much simpler and I cannot see why it wouldn't work.
Ta.
>
> > +
> > + return order;
>
> You sure that return value serves any purpose? Is that convenience so
> you can use the function in a combined statement?
>
Just convenience from splitting the function up.
> > +}
> > +EXPORT_SYMBOL(drm_random_reorder);
> > +
> > +int *drm_random_order(int count, u32 *state)
> > +{
> > + int *order;
> > + int n;
> > +
> > + order = kmalloc_array(count, sizeof(*order), GFP_TEMPORARY);
> > + if (!order)
> > + return order;
> > +
> > + for (n = 0; n < count; n++)
> > + order[n] = n;
> > +
> > + return drm_random_reorder(order, count, state);
>
> Why "signed int"? We very often use "size_t" to count things. By
> making the order signed you just keep running into "comparing signed
> vs unsigned" warnings, don't you?
Because I only needed ints. :-p
>
> > +}
> > +EXPORT_SYMBOL(drm_random_order);
> > diff --git a/drivers/gpu/drm/lib/drm_rand.h b/drivers/gpu/drm/lib/drm_rand.h
> > new file mode 100644
> > index 000000000000..a3f22d115aac
> > --- /dev/null
> > +++ b/drivers/gpu/drm/lib/drm_rand.h
> > @@ -0,0 +1,9 @@
> > +#ifndef __DRM_RAND_H__
> > +#define __DRM_RAND_H
> > +
> > +u32 drm_lcg_random(u32 *state);
> > +
> > +int *drm_random_reorder(int *order, int count, u32 *state);
> > +int *drm_random_order(int count, u32 *state);
> > +
> > +#endif /* __DRM_RAND_H__ */
>
> "random.h". Why the abbreviation?
Before the drm_ prefix I had to avoid a clash and I have a history with
calling this rand.h for no good reason.
> Looks good to me. Only nitpicks, so feel free to ignore.
(Apart from size_t because I still have the burn marks from DRM using
size_t where I need u64 on 32bit kernels. However, that looks correct
for this application. ;)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
