[v2,03/16] domain: add rendernode attribute on <accel>

Message ID a39e7cc3bef1d4fa9031882c7cd620b189990755.1566576129.git.crobinso@redhat.com
State New
Headers show
Series
  • Add vhost-user-gpu support
Related show

Commit Message

Cole Robinson Aug. 23, 2019, 4:21 p.m.
From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost-user-gpu helper may accept --render-node option to specify on
which GPU should the renderning be done.

(by comparison <graphics> rendernode is the target/display rendering)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 docs/formatdomain.html.in     |  5 +++++
 docs/schemas/domaincommon.rng |  5 +++++
 src/conf/domain_conf.c        | 18 +++++++++++++++++-
 src/conf/domain_conf.h        |  1 +
 4 files changed, 28 insertions(+), 1 deletion(-)

Comments

Cole Robinson Aug. 23, 2019, 5:04 p.m. | #1
On 8/23/19 12:21 PM, Cole Robinson wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> vhost-user-gpu helper may accept --render-node option to specify on
> which GPU should the renderning be done.
> 

What does it do if the user doesn't pass one? Pick one itself, or just
not use one somehow?

If it picks one, then we may need to treat this like we treat other
rendernode instances and autoallocate one if the user doesn't specify,
otherwise we won't be able to add the path to the cgroup for example, or
selinux label it if necessary. I'm not sure if that actually applies in
this case, but it's worth considering.

> (by comparison <graphics> rendernode is the target/display rendering)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>

Also I see now that I accidentally signed off all these patches, that
was not intentional. Please strip those from v3

> ---
>  docs/formatdomain.html.in     |  5 +++++
>  docs/schemas/domaincommon.rng |  5 +++++
>  src/conf/domain_conf.c        | 18 +++++++++++++++++-
>  src/conf/domain_conf.h        |  1 +
>  4 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index ec650fbe17..5a4807d937 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7059,6 +7059,11 @@ qemu-kvm -net nic,model=? /dev/null
>          <dd>Enable 3D acceleration (for vbox driver
>          <span class="since">since 0.7.1</span>, qemu driver
>          <span class="since">since 1.3.0</span>)</dd>
> +
> +        <dt><code>rendernode</code></dt>
> +        <dd>Absolute path to a host's DRI device to be used for
> +        rendering (for vhost-user driver only, <span
> +        class="since">since 5.5.0</span>)</dd>
>          </dl>
>        </dd>
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index bac566855d..6e91fe6cef 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3644,6 +3644,11 @@
>                    <ref name="virYesNo"/>
>                  </attribute>
>                </optional>
> +              <optional>
> +                <attribute name="rendernode">
> +                  <ref name="absFilePath"/>
> +                </attribute>
> +              </optional>
>              </element>
>            </optional>
>          </element>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f51575d57d..2cc055491d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2832,6 +2832,8 @@ virDomainVideoDefClear(virDomainVideoDefPtr def)
>  
>      virDomainDeviceInfoClear(&def->info);
>  
> +    if (def->accel)
> +        VIR_FREE(def->accel->rendernode);
>      VIR_FREE(def->accel);
>      VIR_FREE(def->virtio);
>      VIR_FREE(def->driver);
> @@ -15270,6 +15272,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>      int val;
>      VIR_AUTOFREE(char *) accel2d = NULL;
>      VIR_AUTOFREE(char *) accel3d = NULL;
> +    VIR_AUTOFREE(char *) rendernode = NULL;
>  
>      cur = node->children;
>      while (cur != NULL) {
> @@ -15278,12 +15281,13 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>                  virXMLNodeNameEqual(cur, "acceleration")) {
>                  accel3d = virXMLPropString(cur, "accel3d");
>                  accel2d = virXMLPropString(cur, "accel2d");
> +                rendernode = virXMLPropString(cur, "rendernode");
>              }
>          }
>          cur = cur->next;
>      }
>  
> -    if (!accel3d && !accel2d)
> +    if (!accel3d && !accel2d && !rendernode)
>          return NULL;
>  
>      if (VIR_ALLOC(def) < 0)
> @@ -15307,6 +15311,9 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>          def->accel2d = val;
>      }
>  
> +    if (VIR_STRDUP(def->rendernode, rendernode) < 0)
> +        goto cleanup;
> +

Huh, this function has no error reporting? A bogus accel2d/accel3d value
will raise an error but it never triggers the error path in the calling
function. Not your fault of course :)

>   cleanup:
>      return def;
>  }
> @@ -15474,6 +15481,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>              goto error;
>          }
>      }
> +    if (!def->vhostuser && def->accel && def->accel->rendernode) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("unsupported rendernode accel attribute without 'vhost-user'"));
> +        goto error;
> +    }
>  

This function doesn't represent best practices, but this style of check
should be moved to virDomainVideoDefValidate IMO

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Ján Tomko Aug. 27, 2019, 7:32 a.m. | #2
On Fri, Aug 23, 2019 at 01:04:53PM -0400, Cole Robinson wrote:
>On 8/23/19 12:21 PM, Cole Robinson wrote:

>> From: Marc-André Lureau <marcandre.lureau@redhat.com>

>>

>> vhost-user-gpu helper may accept --render-node option to specify on

>> which GPU should the renderning be done.

>>

>

>What does it do if the user doesn't pass one? Pick one itself, or just

>not use one somehow?

>

>If it picks one, then we may need to treat this like we treat other

>rendernode instances and autoallocate one if the user doesn't specify,

>otherwise we won't be able to add the path to the cgroup for example, or

>selinux label it if necessary. I'm not sure if that actually applies in

>this case, but it's worth considering.

>


Even if we'll chose one automatically, we should at least output it in
live XML.

>> (by comparison <graphics> rendernode is the target/display rendering)

>>

>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>

>Also I see now that I accidentally signed off all these patches, that

>was not intentional. Please strip those from v3

>


Per https://libvirt.org/governance.html#codeofconduct we need your
intentional agreement with the Developer Certificate of Origin:
https://developercertificate.org/ to be able to include your changes.

I'd be okay from stripping it from the patches you did not actually
change, but it should stay on those where you actually changed
something.

Also, we do not have that process documented, but the kernel practice
(and what I've been doing) seems to be that the maintainer merging the
patch also adds a sign-off to provide that chain of certificates.

Thirdly, I see some patches committed to libvirt by Marc-André but
no mention in AUTHORS.in history - maybe it's time to get his commit
access restored and documented?

>> ---

>>  docs/formatdomain.html.in     |  5 +++++

>>  docs/schemas/domaincommon.rng |  5 +++++

>>  src/conf/domain_conf.c        | 18 +++++++++++++++++-

>>  src/conf/domain_conf.h        |  1 +

>>  4 files changed, 28 insertions(+), 1 deletion(-)

>>


>> @@ -15474,6 +15481,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,

>>              goto error;

>>          }

>>      }

>> +    if (!def->vhostuser && def->accel && def->accel->rendernode) {

>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

>> +                       _("unsupported rendernode accel attribute without 'vhost-user'"));

>> +        goto error;

>> +    }

>>

>

>This function doesn't represent best practices, but this style of check

>should be moved to virDomainVideoDefValidate IMO

>


Yes.

Jano

>Thanks,

>Cole

>

>--

>libvir-list mailing list

>libvir-list@redhat.com

>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Marc-André Lureau Aug. 30, 2019, 4:14 p.m. | #3
Hi

On Fri, Aug 23, 2019 at 9:05 PM Cole Robinson <crobinso@redhat.com> wrote:
>
> On 8/23/19 12:21 PM, Cole Robinson wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > vhost-user-gpu helper may accept --render-node option to specify on
> > which GPU should the renderning be done.
> >
>
> What does it do if the user doesn't pass one? Pick one itself, or just
> not use one somehow?

This is left unspecified. So it may probe one itself and use it, or
fallback to full-software.

>
> If it picks one, then we may need to treat this like we treat other
> rendernode instances and autoallocate one if the user doesn't specify,
> otherwise we won't be able to add the path to the cgroup for example, or
> selinux label it if necessary. I'm not sure if that actually applies in
> this case, but it's worth considering.
>

I think we may want to auto-allocate a drm node if the helper accepts
the argument, as it is almost always what you'd want. But then we lose
the ability to *not* specify it. I am fine with that.


> > (by comparison <graphics> rendernode is the target/display rendering)
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
>
> Also I see now that I accidentally signed off all these patches, that
> was not intentional. Please strip those from v3
>
> > ---
> >  docs/formatdomain.html.in     |  5 +++++
> >  docs/schemas/domaincommon.rng |  5 +++++
> >  src/conf/domain_conf.c        | 18 +++++++++++++++++-
> >  src/conf/domain_conf.h        |  1 +
> >  4 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index ec650fbe17..5a4807d937 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -7059,6 +7059,11 @@ qemu-kvm -net nic,model=? /dev/null
> >          <dd>Enable 3D acceleration (for vbox driver
> >          <span class="since">since 0.7.1</span>, qemu driver
> >          <span class="since">since 1.3.0</span>)</dd>
> > +
> > +        <dt><code>rendernode</code></dt>
> > +        <dd>Absolute path to a host's DRI device to be used for
> > +        rendering (for vhost-user driver only, <span
> > +        class="since">since 5.5.0</span>)</dd>
> >          </dl>
> >        </dd>
> >
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index bac566855d..6e91fe6cef 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -3644,6 +3644,11 @@
> >                    <ref name="virYesNo"/>
> >                  </attribute>
> >                </optional>
> > +              <optional>
> > +                <attribute name="rendernode">
> > +                  <ref name="absFilePath"/>
> > +                </attribute>
> > +              </optional>
> >              </element>
> >            </optional>
> >          </element>
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index f51575d57d..2cc055491d 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2832,6 +2832,8 @@ virDomainVideoDefClear(virDomainVideoDefPtr def)
> >
> >      virDomainDeviceInfoClear(&def->info);
> >
> > +    if (def->accel)
> > +        VIR_FREE(def->accel->rendernode);
> >      VIR_FREE(def->accel);
> >      VIR_FREE(def->virtio);
> >      VIR_FREE(def->driver);
> > @@ -15270,6 +15272,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
> >      int val;
> >      VIR_AUTOFREE(char *) accel2d = NULL;
> >      VIR_AUTOFREE(char *) accel3d = NULL;
> > +    VIR_AUTOFREE(char *) rendernode = NULL;
> >
> >      cur = node->children;
> >      while (cur != NULL) {
> > @@ -15278,12 +15281,13 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
> >                  virXMLNodeNameEqual(cur, "acceleration")) {
> >                  accel3d = virXMLPropString(cur, "accel3d");
> >                  accel2d = virXMLPropString(cur, "accel2d");
> > +                rendernode = virXMLPropString(cur, "rendernode");
> >              }
> >          }
> >          cur = cur->next;
> >      }
> >
> > -    if (!accel3d && !accel2d)
> > +    if (!accel3d && !accel2d && !rendernode)
> >          return NULL;
> >
> >      if (VIR_ALLOC(def) < 0)
> > @@ -15307,6 +15311,9 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
> >          def->accel2d = val;
> >      }
> >
> > +    if (VIR_STRDUP(def->rendernode, rendernode) < 0)
> > +        goto cleanup;
> > +
>
> Huh, this function has no error reporting? A bogus accel2d/accel3d value
> will raise an error but it never triggers the error path in the calling
> function. Not your fault of course :)
>
> >   cleanup:
> >      return def;
> >  }
> > @@ -15474,6 +15481,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
> >              goto error;
> >          }
> >      }
> > +    if (!def->vhostuser && def->accel && def->accel->rendernode) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("unsupported rendernode accel attribute without 'vhost-user'"));
> > +        goto error;
> > +    }
> >
>
> This function doesn't represent best practices, but this style of check
> should be moved to virDomainVideoDefValidate IMO
>
> Thanks,
> Cole
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index ec650fbe17..5a4807d937 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7059,6 +7059,11 @@  qemu-kvm -net nic,model=? /dev/null
         <dd>Enable 3D acceleration (for vbox driver
         <span class="since">since 0.7.1</span>, qemu driver
         <span class="since">since 1.3.0</span>)</dd>
+
+        <dt><code>rendernode</code></dt>
+        <dd>Absolute path to a host's DRI device to be used for
+        rendering (for vhost-user driver only, <span
+        class="since">since 5.5.0</span>)</dd>
         </dl>
       </dd>
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bac566855d..6e91fe6cef 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3644,6 +3644,11 @@ 
                   <ref name="virYesNo"/>
                 </attribute>
               </optional>
+              <optional>
+                <attribute name="rendernode">
+                  <ref name="absFilePath"/>
+                </attribute>
+              </optional>
             </element>
           </optional>
         </element>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f51575d57d..2cc055491d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2832,6 +2832,8 @@  virDomainVideoDefClear(virDomainVideoDefPtr def)
 
     virDomainDeviceInfoClear(&def->info);
 
+    if (def->accel)
+        VIR_FREE(def->accel->rendernode);
     VIR_FREE(def->accel);
     VIR_FREE(def->virtio);
     VIR_FREE(def->driver);
@@ -15270,6 +15272,7 @@  virDomainVideoAccelDefParseXML(xmlNodePtr node)
     int val;
     VIR_AUTOFREE(char *) accel2d = NULL;
     VIR_AUTOFREE(char *) accel3d = NULL;
+    VIR_AUTOFREE(char *) rendernode = NULL;
 
     cur = node->children;
     while (cur != NULL) {
@@ -15278,12 +15281,13 @@  virDomainVideoAccelDefParseXML(xmlNodePtr node)
                 virXMLNodeNameEqual(cur, "acceleration")) {
                 accel3d = virXMLPropString(cur, "accel3d");
                 accel2d = virXMLPropString(cur, "accel2d");
+                rendernode = virXMLPropString(cur, "rendernode");
             }
         }
         cur = cur->next;
     }
 
-    if (!accel3d && !accel2d)
+    if (!accel3d && !accel2d && !rendernode)
         return NULL;
 
     if (VIR_ALLOC(def) < 0)
@@ -15307,6 +15311,9 @@  virDomainVideoAccelDefParseXML(xmlNodePtr node)
         def->accel2d = val;
     }
 
+    if (VIR_STRDUP(def->rendernode, rendernode) < 0)
+        goto cleanup;
+
  cleanup:
     return def;
 }
@@ -15474,6 +15481,11 @@  virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
             goto error;
         }
     }
+    if (!def->vhostuser && def->accel && def->accel->rendernode) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("unsupported rendernode accel attribute without 'vhost-user'"));
+        goto error;
+    }
 
     if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
         goto error;
@@ -26452,6 +26464,10 @@  virDomainVideoAccelDefFormat(virBufferPtr buf,
         virBufferAsprintf(buf, " accel2d='%s'",
                           virTristateBoolTypeToString(def->accel2d));
     }
+    if (def->rendernode) {
+        virBufferAsprintf(buf, " rendernode='%s'",
+                          def->rendernode);
+    }
     virBufferAddLit(buf, "/>\n");
 }
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bc2450f25e..707fbd1cd3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1405,6 +1405,7 @@  VIR_ENUM_DECL(virDomainVideoVGAConf);
 struct _virDomainVideoAccelDef {
     int accel2d; /* enum virTristateBool */
     int accel3d; /* enum virTristateBool */
+    char *rendernode;
 };