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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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: OpenPGP_signature.asc
Description: OpenPGP digital signature