Re: [PATCH v2 3/5] drm/simpledrm: Store xfrm buffer in device instance

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3919 bytes --]

Hi

Am 26.09.23 um 09:31 schrieb Jocelyn Falempe:
> On 20/09/2023 16:24, Thomas Zimmermann wrote:
>> Store and instance of struct drm_xfrm_buf in struct simpledrm_device
>> and keep the allocated memory allocated across display updates. Avoid
>> possibly reallocating temporary memory on each display update. Instead
>> preallocate temporary memory during initialization. Releasing the DRM
>> device also releases the xfrm buffer.
>>
>> v2:
>>     * reserve storage during probe
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tiny/simpledrm.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c 
>> b/drivers/gpu/drm/tiny/simpledrm.c
>> index 8aceb7d378dea..a3d8a956a4c4e 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -232,6 +232,7 @@ struct simpledrm_device {
>>       struct drm_display_mode mode;
>>       const struct drm_format_info *format;
>>       unsigned int pitch;
>> +    struct drm_xfrm_buf xfrm;
>>       /* memory management */
>>       struct iosys_map screen_base;
>> @@ -486,7 +487,6 @@ static void 
>> simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>>       struct drm_framebuffer *fb = plane_state->fb;
>>       struct drm_device *dev = plane->dev;
>>       struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>> -    struct drm_xfrm_buf xfrm = DRM_XFRM_BUF_INIT;
>>       struct drm_atomic_helper_damage_iter iter;
>>       struct drm_rect damage;
>>       int ret, idx;
>> @@ -508,13 +508,12 @@ static void 
>> simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>>           iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, 
>> sdev->format, &dst_clip));
>>           drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, 
>> shadow_plane_state->data,
>> -                fb, &damage, &xfrm);
>> +                fb, &damage, &sdev->xfrm);
>>       }
>>       drm_dev_exit(idx);
>>   out_drm_gem_fb_end_cpu_access:
>>       drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> -    drm_xfrm_buf_release(&xfrm);
>>   }
>>   static void simpledrm_primary_plane_helper_atomic_disable(struct 
>> drm_plane *plane,
>> @@ -637,6 +636,7 @@ static struct simpledrm_device 
>> *simpledrm_device_create(struct drm_driver *drv,
>>       struct drm_connector *connector;
>>       unsigned long max_width, max_height;
>>       size_t nformats;
>> +    void *buf;
>>       int ret;
>>       sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct 
>> simpledrm_device, dev);
>> @@ -718,6 +718,13 @@ static struct simpledrm_device 
>> *simpledrm_device_create(struct drm_driver *drv,
>>       drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d 
>> byte\n",
>>           &format->format, width, height, stride);
>> +    ret = drmm_xfrm_buf_init(dev, &sdev->xfrm);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +    buf = drm_xfrm_buf_reserve(&sdev->xfrm, sdev->pitch, GFP_KERNEL);
>> +    if (!buf)
>> +        return ERR_PTR(-ENOMEM);
>> +
> 
> I think it would be nice to have a "init_and_reserve()" function, to 
> simplify the callers ?

I think I can add the reserve parameters directly to the _init() 
function. Reserving '0' will then not reserve memory.

Best regards
Thomas

> 
>>       /*
>>        * Memory management
>>        */
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]