Re: [Embedded-pv-devel] [PATCH] drmif: add ABI for para-virtual DRM/KMS

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.11.16 at 12:30, <andr2000@xxxxxxxxx> wrote:
> --- /dev/null
> +++ b/include/xen/interface/io/drmif_linux.h
> @@ -0,0 +1,142 @@
> +/******************************************************************************
> + * drmif_linux.h
> + *
> + *  Unified DRM-device I/O interface for Xen guest OSes
> + *
> + * 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 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.
> + *
> + * Copyright (C) 2016 EPAM Systems Inc.
> + *
> + * Authors: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
> + *          Oleksandr Grytsov <al1img@xxxxxxxxx>
> + */
> +
> +#ifndef __XEN_PUBLIC_IO_XENSND_LINUX_H__
> +#define __XEN_PUBLIC_IO_XENSND_LINUX_H__
> +
> +#include <xen/interface/io/ring.h>
> +#include <xen/interface/io/drmif.h>
> +#include <xen/interface/grant_table.h>
> +
> +struct xendrm_dumb_create_req {
> +     uint64_t dumb_cookie;
> +     uint32_t width;
> +     uint32_t height;
> +     uint32_t bpp;
> +     grant_ref_t gref_directory_start;
> +} __packed;

So looking a little more closely at the split here - what's the Linux
specific part of the above structure? I.e. perhaps my sndif related
comment was then also wrong, and instead you mean to just have
everything in a single file? At least that's what it looks like here.
May I suggest that you try to follow the fundamental model the
other, pre-existing interface headers use? And along that line
please keep in mind that frontend and backend may anyway be
running in different OSes, so the split done here isn't really
architecturally correct.

Also please note that __packed is undefined here.

> +struct xendrm_req {
> +     union {
> +             struct xendrm_request raw;
> +             struct {
> +                     uint16_t id;
> +                     uint8_t operation;
> +                     uint8_t reserved;
> +                     union {
> +                             struct xendrm_dumb_create_req dumb_create;
> +                             struct xendrm_dumb_destroy_req dumb_destroy;
> +                             struct xendrm_fb_create_req fb_create;
> +                             struct xendrm_fb_destroy_req fb_destroy;
> +                             struct xendrm_set_config_req set_config;
> +                             struct xendrm_page_flip_req pg_flip;
> +                     } op;
> +             } data __packed;
> +     } u;
> +};

A structure whose only member is a union is odd - please make this
a union itself.

Jan


_______________________________________________
Embedded-pv-devel mailing list
Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel