Re: [PATCH 15/17] drm/vmwgfx: Add surface define v4 command

Am 23.03.20 um 13:02 schrieb Emil Velikov:

Hi all,

Just a small fly-by idea.

On Thu, 19 Mar 2020 at 20:25, rscheidegger.oss@gmail.com wrote:

...

From: Deepak Rawat drawat.floss@gmail.com

Surface define v4 added new member buffer_byte_stride. With this patch add buffer_byte_stride in surface metadata and create surface using new command if support is available.

Also with this patch replace device specific data types with kernel types.

Signed-off-by: Deepak Rawat drawat.floss@gmail.com Reviewed-by: Thomas Hellström (VMware) thomas_os@shipmail.org Signed-off-by: Roland Scheidegger (VMware) rscheidegger.oss@gmail.com

---

drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 ++ drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 36 +++++++++++++++++++++++-- include/uapi/drm/vmwgfx_drm.h | 12 +++++---- 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index 212b66da2d4c..aa4131f5f8fc 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -238,6 +238,7 @@ struct vmw_surface_offset;

• @array_size: Number of array elements for a 1D/2D texture. For cubemap texture number of faces * array_size. This should be 0 for pre SM4 device.

• • @buffer_byte_stride: Buffer byte stride.
  • @num_sizes: Size of @sizes. For GB surface this should always be 1.
  • @base_size: Surface dimension.
  • @sizes: Array representing mip sizes. Legacy only.

@@ -255,6 +256,7 @@ struct vmw_surface_metadata { u32 autogen_filter; u32 array_size; u32 num_sizes;

•   u32 buffer_byte_stride;
    struct drm_vmw_size base_size;
    struct drm_vmw_size *sizes;
    bool scanout;
  

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 0e71a3ea281b..dad49ab4a0bf 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -1082,6 +1082,10 @@ static int vmw_gb_surface_create(struct vmw_resource *res) SVGA3dCmdHeader header; SVGA3dCmdDefineGBSurface_v3 body; } *cmd3;

•   struct {
  

•           SVGA3dCmdHeader header;
  

•           SVGA3dCmdDefineGBSurface_v4 body;
  

•   } *cmd4;
  
    if (likely(res->id != -1))
            return 0;
  

@@ -1098,7 +1102,11 @@ static int vmw_gb_surface_create(struct vmw_resource *res) goto out_no_fifo; }

•   if (has_sm4_1_context(dev_priv) && metadata->array_size > 0) {
  

•   if (has_sm5_context(dev_priv) && metadata->array_size > 0) {
  

•           cmd_id = SVGA_3D_CMD_DEFINE_GB_SURFACE_V4;
  

•           cmd_len = sizeof(cmd4->body);
  

•           submit_len = sizeof(*cmd4);
  

•   } else if (has_sm4_1_context(dev_priv) && metadata->array_size > 0) {
            cmd_id = SVGA_3D_CMD_DEFINE_GB_SURFACE_V3;
            cmd_len = sizeof(cmd3->body);
            submit_len = sizeof(*cmd3);
  

@@ -1116,12 +1124,29 @@ static int vmw_gb_surface_create(struct vmw_resource *res) cmd = VMW_FIFO_RESERVE(dev_priv, submit_len); cmd2 = (typeof(cmd2))cmd; cmd3 = (typeof(cmd3))cmd;

•   cmd4 = (typeof(cmd4))cmd;
    if (unlikely(!cmd)) {
            ret = -ENOMEM;
            goto out_no_fifo;
    }
  
  

I'm not sure if SVGA3dCmdDefineGBSurface_** is ABI or not. If not one could tweak and simplify things a fair bit:

Yes, these are part of the svga device protocol, so the struct layout is non-tweakable. v2 actually has the same layout as v1, with just 2 new members at the end, v4 has the same layout as v3, with one new member - but unfortunately v3 doesn't have the same layout as v2. So since it could only work in pairs I'm not sure it's worth changing, and actually I'm not entirely sure it's ok if we reserve/submit more dats than necessary?

Roland

• tweak the struct layout - always append newly introduced variables
• allocate the max size (in VMW_FIFO_RESERVE)
• cut down the massive duplication as seen below

cmd4->foo = foo... cmd3->foo = foo... cmd2->foo = foo... cmd->foo = foo

...

•   if (has_sm4_1_context(dev_priv) && metadata->array_size > 0) {
  

•   if (has_sm5_context(dev_priv) && metadata->array_size > 0) {
  

•           cmd4->header.id = cmd_id;
  

•           cmd4->header.size = cmd_len;
  

•           cmd4->body.sid = srf->res.id;
  

•           cmd4->body.surfaceFlags = metadata->flags;
  

•           cmd4->body.format = metadata->format;
  

•           cmd4->body.numMipLevels = metadata->mip_levels[0];
  

•           cmd4->body.multisampleCount = metadata->multisample_count;
  

•           cmd4->body.multisamplePattern = metadata->multisample_pattern;
  

•           cmd4->body.qualityLevel = metadata->quality_level;
  

•           cmd4->body.autogenFilter = metadata->autogen_filter;
  

•           cmd4->body.size.width = metadata->base_size.width;
  

•           cmd4->body.size.height = metadata->base_size.height;
  

•           cmd4->body.size.depth = metadata->base_size.depth;
  

•           cmd4->body.arraySize = metadata->array_size;
  

•           cmd4->body.bufferByteStride = metadata->buffer_byte_stride;
  

•   } else if (has_sm4_1_context(dev_priv) && metadata->array_size > 0) {
            cmd3->header.id = cmd_id;
            cmd3->header.size = cmd_len;
            cmd3->body.sid = srf->res.id;
  

HTH Emil