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

[Emil Velikov]

On 26 January 2018 at 17:27, Matt Roper  wrote:
> On Fri, Jan 26, 2018 at 05:08:48PM +, Emil Velikov wrote:
>> On 22 January 2018 at 15:44, Matt Roper  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 
>> > ---
>> >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Fri, Jan 26, 2018 at 05:08:48PM +, Emil Velikov wrote:
> On 22 January 2018 at 15:44, Matt Roper  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 
> > ---
> >  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.

> 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).

Thanks for the feedback!

Matt

> 
> Thanks
> Emil
> 
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt?h=v4.15-rc9#n64

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Emil Velikov]

On 22 January 2018 at 15:44, Matt Roper  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 
> ---
>  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.

There's a small suggestions about the IOCTL design.
s/reserved/flags/ to make future extension are possible - as mentioned in [2]

Thanks
Emil

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt?h=v4.15-rc9#n64
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx