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

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:

- 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