From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gerd Hoffmann Subject: Re: [PATCH v2 4/6] virtio-gpu: add 3d/virgl support Date: Tue, 22 Sep 2015 17:18:18 +0200 Message-ID: <1442935098.13084.31.camel@redhat.com> References: <1442828417-6165-1-git-send-email-kraxel@redhat.com> <1442828417-6165-5-git-send-email-kraxel@redhat.com> <20150921130110-mutt-send-email-mst@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20150921130110-mutt-send-email-mst@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: "Michael S. Tsirkin" Cc: "open list:ABI/API" , open list , "open list:VIRTIO GPU DRIVER" , "open list:VIRTIO GPU DRIVER" , Dave Airlie List-Id: linux-api@vger.kernel.org ICBIaSwKCj4gPiAgc3RhdGljIHVuc2lnbmVkIGludCBmZWF0dXJlc1tdID0gewo+ID4gKyNpZmRl ZiBfX0xJVFRMRV9FTkRJQU4KPiA+ICsJVklSVElPX0dQVV9GRUFUVVJFX1ZJUkdMLAo+ID4gKyNl bmRpZgo+ID4gIH07Cj4gCj4gV2h5IGlzIHZpcmdsIExFIHNwZWNpZmljPyBKdXN0IGN1cmlvdXMu CgpnYWxsaXVtIGNvbW1hbmQgc3RyZWFtIGlzIG5hdGl2ZSBlbmRpYW4sIGFuZCB3ZSBvbmx5IHN1 cHBvcnQgbGl0dGxlCmVuZGlhbiBndWVzdHMgb24gbGl0dGxlIGVuZGlhbiBob3N0cyBmb3IgdGhh dCByZWFzb24uCgpTdXBwb3J0aW5nIGJpZ2VuZGlhbiBndWVzdHMgb24gYmlnZW5kaWFuIGhvc3Rz IHNob3VsZCBiZSBwb3NzaWJsZSB0b28KKHdpdGggYSBWSVJHbF9CRSBmZWF0dXJlIGZsYWcpLCBi dXQgd2l0aCBldmVuIHRoZSBwb3dlcnBjIHdvcmxkIG1vdmluZwp0byBsaXR0bGUgZW5kaWFuIEkg c2VlIGxpdHRsZSByZWFzb24gdG8gYWN0dWFsbHkgYm90aGVyLgoKVG8gYmUgY2xlYXI6IHRoaXMg YWZmZWN0cyAzZCBhY2NlbGVyYXRpb24gb25seSwgMmQgbW9kZSB3b3JrcyBmaW5lIG9uCmV2ZXJ5 IGhvc3QvZ3Vlc3QgZW5kaWFuIGNvbWJpbmF0aW9uLgoKPiA+ICsjaWZkZWYgX19MSVRUTEVfRU5E SUFOCj4gPiArCWlmICh2aXJ0aW9faGFzX2ZlYXR1cmUodmdkZXYtPnZkZXYsIFZJUlRJT19HUFVf RkVBVFVSRV9WSVJHTCkpCj4gPiArCQl2Z2Rldi0+aGFzX3ZpcmdsXzNkID0gdHJ1ZTsKPiA+ICsj ZW5kaWYKPiAKPiBZb3UgbWlnaHQgYmUgYWJsZSB0byBnZXQgYnkgd2l0aG91dCB0aGlzIGlmZGVm IGFzCj4gVklSVElPX0dQVV9GRUFUVVJFX1ZJUkdMIHdvbid0IGJlIHNldCB3aXRob3V0IGl0LgoK WWVzLCByaWdodCwgdGhpcyAjaWZkZWYgY2FuIGdvIGF3YXkuCgo+ID4gK2ludCB2aXJ0aW9fZ3B1 X2NtZF9nZXRfY2Fwc2V0X2luZm8oc3RydWN0IHZpcnRpb19ncHVfZGV2aWNlICp2Z2RldiwgaW50 IGlkeCkKPiA+ICt7Cj4gPiArCXN0cnVjdCB2aXJ0aW9fZ3B1X2dldF9jYXBzZXRfaW5mbyAqY21k X3A7Cj4gPiArCXN0cnVjdCB2aXJ0aW9fZ3B1X3ZidWZmZXIgKnZidWY7Cj4gPiArCXZvaWQgKnJl c3BfYnVmOwo+ID4gKwo+ID4gKwlyZXNwX2J1ZiA9IGt6YWxsb2Moc2l6ZW9mKHN0cnVjdCB2aXJ0 aW9fZ3B1X3Jlc3BfY2Fwc2V0X2luZm8pLAo+ID4gKwkJCSAgIEdGUF9LRVJORUwpOwo+ID4gKwlp ZiAoIXJlc3BfYnVmKQo+ID4gKwkJcmV0dXJuIC1FTk9NRU07Cj4gPiArCj4gPiArCWNtZF9wID0g dmlydGlvX2dwdV9hbGxvY19jbWRfcmVzcAo+ID4gKwkJKHZnZGV2LCAmdmlydGlvX2dwdV9jbWRf Z2V0X2NhcHNldF9pbmZvX2NiLCAmdmJ1ZiwKPiA+ICsJCSBzaXplb2YoKmNtZF9wKSwgc2l6ZW9m KHN0cnVjdCB2aXJ0aW9fZ3B1X3Jlc3BfY2Fwc2V0X2luZm8pLAo+ID4gKwkJIHJlc3BfYnVmKTsK PiA+ICsJbWVtc2V0KGNtZF9wLCAwLCBzaXplb2YoKmNtZF9wKSk7Cj4gPiArCj4gPiArCWNtZF9w LT5oZHIudHlwZSA9IGNwdV90b19sZTMyKFZJUlRJT19HUFVfQ01EX0dFVF9DQVBTRVRfSU5GTyk7 Cj4gPiArCWNtZF9wLT5jYXBzZXRfaW5kZXggPSBjcHVfdG9fbGUzMihpZHgpOwo+ID4gKwl2aXJ0 aW9fZ3B1X3F1ZXVlX2N0cmxfYnVmZmVyKHZnZGV2LCB2YnVmKTsKPiA+ICsJcmV0dXJuIDA7Cj4g PiArfQo+ID4gKwo+IAo+IEkgbm90ZSB0aGF0IGFsbCBjYWxsZXJzIGlnbm9yZSB0aGUgcmVjdXJu IGNvZGUgZnJvbQo+IHZpcnRpb19ncHVfcXVldWVfY3RybF9idWZmZXIuCj4gCj4gSXMgdGhlcmUg YSByZWFzb24gaXQgY2FuJ3QgZmFpbD8KCkl0IGNhbiBvbmx5IGZhaWwgd2hlbiB2Z2Rldi0+dnFz X3JlYWR5IGlzIGZhbHNlLiAgV2hpY2ggY2FuIG9ubHkgaGFwcGVuCndoZW4gd2UgYXJlIGFib3V0 IHRvIHVubG9hZCB0aGUgZHJpdmVyLiAgSW4gdGhhdCBjYXNlIHdlIHByb2JhYmx5IGRvbid0Cndv cnJ5IG11Y2ggYWJvdXQgdGhlIGZhaWx1cmUgLi4uCgo+IFNob3VsZG4ndCBpdCBiZSBtYWRlIHZv aWQgdGhlbj8KCkkgZ3Vlc3Mgd2Ugc2hvdWxkIC4uLgoKPiA+ICsJY21kX3AtPmhkci50eXBlID0g Y3B1X3RvX2xlMzIoVklSVElPX0dQVV9DTURfQ1RYX0NSRUFURSk7Cj4gPiArCWNtZF9wLT5oZHIu Y3R4X2lkID0gY3B1X3RvX2xlMzIoaWQpOwo+ID4gKwljbWRfcC0+bmxlbiA9IGNwdV90b19sZTMy KG5sZW4pOwo+ID4gKwlzdHJuY3B5KGNtZF9wLT5kZWJ1Z19uYW1lLCBuYW1lLCA2Myk7Cj4gPiAr CWNtZF9wLT5kZWJ1Z19uYW1lWzYzXSA9IDA7Cj4gCj4gc2l6ZW9mIChjbWRfcC0+ZGVidWdfbmFt ZSkgLSAxIHdpbGwgYmUgYSBidXQgcHJldHRpZXIuCgpBZ3JlZS4KCj4gPiBkaWZmIC0tZ2l0IGEv aW5jbHVkZS91YXBpL2xpbnV4L3ZpcnRpb19ncHUuaCBiL2luY2x1ZGUvdWFwaS9saW51eC92aXJ0 aW9fZ3B1LmgKPiA+IGluZGV4IDQ3OGJlNTIuLjdmNGY5Y2UgMTAwNjQ0Cj4gPiAtLS0gYS9pbmNs dWRlL3VhcGkvbGludXgvdmlydGlvX2dwdS5oCj4gPiArKysgYi9pbmNsdWRlL3VhcGkvbGludXgv dmlydGlvX2dwdS5oCj4gPiBAQCAtNDAsNiArNDAsOCBAQAo+ID4gIAo+ID4gICNpbmNsdWRlIDxs aW51eC90eXBlcy5oPgo+ID4gIAo+ID4gKyNkZWZpbmUgVklSVElPX0dQVV9GRUFUVVJFX1ZJUkdM IDAKPiA+ICsKPiAKPiBJJ2QgcHJlZmVyIGl0IG5hbWVkIFZJUlRJT19HUFVfRl9WSVJHTCBmb3Ig Y29uc2lzdGVuY3kgd2l0aCBvdGhlcgo+IGRldmljZXMuCgpNYWtlcyBzZW5zZSwgSSdsbCBjaGFu Z2UgaXQuCgp0aGFua3MsCiAgR2VyZAoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZy ZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGlu Zm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758202AbbIVPSY (ORCPT ); Tue, 22 Sep 2015 11:18:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41883 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757787AbbIVPSW (ORCPT ); Tue, 22 Sep 2015 11:18:22 -0400 Message-ID: <1442935098.13084.31.camel@redhat.com> Subject: Re: [PATCH v2 4/6] virtio-gpu: add 3d/virgl support From: Gerd Hoffmann To: "Michael S. Tsirkin" Cc: David Airlie , Dave Airlie , open list , "open list:VIRTIO GPU DRIVER" , "open list:VIRTIO GPU DRIVER" , "open list:ABI/API" Date: Tue, 22 Sep 2015 17:18:18 +0200 In-Reply-To: <20150921130110-mutt-send-email-mst@redhat.com> References: <1442828417-6165-1-git-send-email-kraxel@redhat.com> <1442828417-6165-5-git-send-email-kraxel@redhat.com> <20150921130110-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, > > static unsigned int features[] = { > > +#ifdef __LITTLE_ENDIAN > > + VIRTIO_GPU_FEATURE_VIRGL, > > +#endif > > }; > > Why is virgl LE specific? Just curious. gallium command stream is native endian, and we only support little endian guests on little endian hosts for that reason. Supporting bigendian guests on bigendian hosts should be possible too (with a VIRGl_BE feature flag), but with even the powerpc world moving to little endian I see little reason to actually bother. To be clear: this affects 3d acceleration only, 2d mode works fine on every host/guest endian combination. > > +#ifdef __LITTLE_ENDIAN > > + if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_FEATURE_VIRGL)) > > + vgdev->has_virgl_3d = true; > > +#endif > > You might be able to get by without this ifdef as > VIRTIO_GPU_FEATURE_VIRGL won't be set without it. Yes, right, this #ifdef can go away. > > +int virtio_gpu_cmd_get_capset_info(struct virtio_gpu_device *vgdev, int idx) > > +{ > > + struct virtio_gpu_get_capset_info *cmd_p; > > + struct virtio_gpu_vbuffer *vbuf; > > + void *resp_buf; > > + > > + resp_buf = kzalloc(sizeof(struct virtio_gpu_resp_capset_info), > > + GFP_KERNEL); > > + if (!resp_buf) > > + return -ENOMEM; > > + > > + cmd_p = virtio_gpu_alloc_cmd_resp > > + (vgdev, &virtio_gpu_cmd_get_capset_info_cb, &vbuf, > > + sizeof(*cmd_p), sizeof(struct virtio_gpu_resp_capset_info), > > + resp_buf); > > + memset(cmd_p, 0, sizeof(*cmd_p)); > > + > > + cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_GET_CAPSET_INFO); > > + cmd_p->capset_index = cpu_to_le32(idx); > > + virtio_gpu_queue_ctrl_buffer(vgdev, vbuf); > > + return 0; > > +} > > + > > I note that all callers ignore the recurn code from > virtio_gpu_queue_ctrl_buffer. > > Is there a reason it can't fail? It can only fail when vgdev->vqs_ready is false. Which can only happen when we are about to unload the driver. In that case we probably don't worry much about the failure ... > Shouldn't it be made void then? I guess we should ... > > + cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_CTX_CREATE); > > + cmd_p->hdr.ctx_id = cpu_to_le32(id); > > + cmd_p->nlen = cpu_to_le32(nlen); > > + strncpy(cmd_p->debug_name, name, 63); > > + cmd_p->debug_name[63] = 0; > > sizeof (cmd_p->debug_name) - 1 will be a but prettier. Agree. > > diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h > > index 478be52..7f4f9ce 100644 > > --- a/include/uapi/linux/virtio_gpu.h > > +++ b/include/uapi/linux/virtio_gpu.h > > @@ -40,6 +40,8 @@ > > > > #include > > > > +#define VIRTIO_GPU_FEATURE_VIRGL 0 > > + > > I'd prefer it named VIRTIO_GPU_F_VIRGL for consistency with other > devices. Makes sense, I'll change it. thanks, Gerd