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

[Roland Scheidegger <rscheidegger.oss@gmail.com>]

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
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel