Thomas Zimmermann <tzimmermann@xxxxxxx> writes: > Store and instance of struct drm_xfrm_buf in struct ssd130x_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> > --- [...] > @@ -1084,6 +1081,8 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) > struct ssd130x_device *ssd130x; > struct backlight_device *bl; > struct drm_device *drm; > + const struct drm_format_info *fi; > + void *buf; > int ret; > > ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver, > @@ -1117,6 +1116,18 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) > bl->props.max_brightness = MAX_CONTRAST; > ssd130x->bl_dev = bl; > > + ret = drmm_xfrm_buf_init(drm, &ssd130x->xfrm); > + if (ret) > + return ERR_PTR(ret); > + fi = drm_format_info(DRM_FORMAT_R1); > + if (!fi) > + return ERR_PTR(-EINVAL); > + buf = drm_xfrm_buf_reserve(&ssd130x->xfrm, > + drm_format_info_min_pitch(fi, 0, ssd130x->width), > + GFP_KERNEL); > + if (!buf) > + return ERR_PTR(-ENOMEM); > + I think this is OK but then I wonder if we should not just allocate all the buffers in the probe function. Right now, what the driver does is to have two structures to keep the driver-private atomic state: 1) struct ssd130x_crtc_state that has a .data_array to store the pixels in the HW format (e.g: R1) and written to the panel. 2) struct ssd130x_plane_state that has a .buffer to store the pixels that are converted from the emulated XRGB8888 used by the shadow-plane, to the HW pixel format. The (2) will be optional once Geert's R1 support lands. Now we are adding a third buffer so I wonder if should be part of one of these private state or not. I said that should be a field of struct ssd130x_plane_state in a previous email, but on a second thought it makes more sense if is a field of the struct ssd130x_crtc_state. That way the allocation will only be in ssd130x_crtc_atomic_check() and the release in the ssd130x_crtc_destroy_state(). If you do that on patch #2, then this patch #5 could be dropped. The reason why I added those private state structures is twofold: because the buffers are tied to the CRTC and planes and to show how a driver can maintain their own private atomic state. After all, one of my goals of this driver is to be used for educational purposes and provide a simple driver that people can use as a reference. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
