[Intel-gfx] [PATCH libdrm] tests: Add drm_set_cgrp_param

[Emil Velikov]

On 26 January 2018 at 17:27, Matt Roper <matthew.d.roper at intel.com> wrote:
> On Fri, Jan 26, 2018 at 05:08:48PM +0000, Emil Velikov wrote:
>> On 22 January 2018 at 15:44, Matt Roper <matthew.d.roper at intel.com> wrote:
>> > drm_set_cgrp_param is a simple tool to set DRM parameters associated with a
>> > cgroup.  It is intended to be called at system initialization time (e.g., from
>> > a sysv-init script or systemd service) to configure graphics policy and
>> > resource management according to the wishes of the system integrator.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> > ---
>> >  configure.ac                                  |  1 +
>> >  tests/Makefile.am                             |  2 +-
>> >  tests/drm_set_cgrp_param/Makefile.am          | 18 ++++++
>> >  tests/drm_set_cgrp_param/drm_set_cgrp_param.c | 80 +++++++++++++++++++++++++++
>> >  4 files changed, 100 insertions(+), 1 deletion(-)
>> >  create mode 100644 tests/drm_set_cgrp_param/Makefile.am
>> >  create mode 100644 tests/drm_set_cgrp_param/drm_set_cgrp_param.c
>> >
>> Hi Matt,
>>
>> Adding a small test/demo in libdrm sounds good. Although I think we
>> need some IGT tests, if you haven't prepped them already.
>> After all we need to ensure the kernel correctly validates and errors
>> when we feed it the wrong info through the IOCTL.
>
> Yeah, agreed.  Writing real IGT's was in the TODO list of the
> cover-letter for my kernel patch series.  This kernel work is still a
> pretty early RFC just to gather feedback on the general approach of
> integrating cgroups with DRM; I figured I'd wait on writing real IGT's
> until we're more confident that this is the right approach in general.
>
> I'm working on a v2 right now that makes some pretty significant changes
> to the series, but I'm not sure yet whether the uapi will change in my
> next iteration or not.
>
Good call - have an agreement about the interface and usage first.
Then iron out all the fiddly bits.

>> There's a small suggestions about the IOCTL design.
>> s/reserved/flags/ to make future extension are possible - as mentioned in [2]
>
> Yeah, that's why I added the reserved field; we don't have any actual
> flags yet, but as soon as we do we'd rename the field to flags and
> document it accordingly.  I can rename the field immediately if you
> think that's easier.  I think the most important thing that's missing at
> the moment is that the kernel patches forgot to check that the reserved
> field is actually empty (i.e., we should reject calls with any garbage
> in there now so that we don't break ABI in the future when we start
> really using those bits for something).
>
Please rename it now. Otherwise we'll get a build break for new kernel
and old userspace.
And yes, validating (returning -EINVAL IIRC) the flags is a must.

Thanks
Emil