Re: [PATCH v3 2/6] drm/tinydrm: generalize tinydrm_xrgb8888_to_gray8()

Noralf Trønnes
2017-08-04T13:20:45Z

Den 04.08.2017 09.27, skrev Noralf Trønnes:
>
> Den 04.08.2017 00.33, skrev David Lechner:
>> This adds parameters for vaddr and clip to
>> tinydrm_xrgb8888_to_gray8() to
>> make it more generic.
>>
>> dma_buf_{begin,end}_cpu_access() are moved out to the repaper driver.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 35
>> ++++++++++----------------
>> drivers/gpu/drm/tinydrm/repaper.c | 21 +++++++++++++++-
>> include/drm/tinydrm/tinydrm-helpers.h | 3 ++-
>> 3 files changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> index 75808bb..5915ba8 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> @@ -185,7 +185,9 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>> /**
>> * tinydrm_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
>> * @dst: 8-bit grayscale destination buffer
>> + * @vaddr: XRGB8888 source buffer
>> * @fb: DRM framebuffer
>> + * @clip: Clip rectangle area to copy
>> *
>> * Drm doesn't have native monochrome or grayscale support.
>> * Such drivers can announce the commonly supported XR24 format to
>> userspace
>> @@ -199,12 +201,11 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>> * Returns:
>> * Zero on success, negative error code on failure.
>> */
>> -int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
>> +int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct
>> drm_framebuffer *fb,
>> + struct drm_clip_rect *clip)
>> {
>> - struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>> - struct dma_buf_attachment *import_attach =
>> cma_obj->base.import_attach;
>> - unsigned int x, y, pitch = fb->pitches[0];
>> - int ret = 0;
>> + unsigned int len = (clip->x2 - clip->x1) * sizeof(u32);
>> + unsigned int x, y;
>> void *buf;
>> u32 *src;
>> @@ -214,22 +215,16 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct
>> drm_framebuffer *fb)
>> * The cma memory is write-combined so reads are uncached.
>> * Speed up by fetching one line at a time.
>> */
>> - buf = kmalloc(pitch, GFP_KERNEL);
>> + buf = kmalloc(len, GFP_KERNEL);
>> if (!buf)
>> return -ENOMEM;
>> - if (import_attach) {
>> - ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
>> - DMA_FROM_DEVICE);
>> - if (ret)
>> - goto err_free;
>> - }
>> -
>> - for (y = 0; y < fb->height; y++) {
>> - src = cma_obj->vaddr + (y * pitch);
>> - memcpy(buf, src, pitch);
>> + for (y = clip->y1; y < clip->y2; y++) {
>> + src = vaddr + (y * fb->pitches[0]);
>> + src += clip->x1;
>> + memcpy(buf, src, len);
>> src = buf;
>> - for (x = 0; x < fb->width; x++) {
>> + for (x = clip->x1; x < clip->x2; x++) {
>> u8 r = (*src & 0x00ff0000) >> 16;
>> u8 g = (*src & 0x0000ff00) >> 8;
>> u8 b = *src & 0x000000ff;
>> @@ -240,13 +235,9 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct
>> drm_framebuffer *fb)
>> }
>> }
>> - if (import_attach)
>> - ret = dma_buf_end_cpu_access(import_attach->dmabuf,
>> - DMA_FROM_DEVICE);
>> -err_free:
>> kfree(buf);
>> - return ret;
>> + return 0;
>> }
>> EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>> diff --git a/drivers/gpu/drm/tinydrm/repaper.c
>> b/drivers/gpu/drm/tinydrm/repaper.c
>> index 3343d3f..d34cd9b 100644
>> --- a/drivers/gpu/drm/tinydrm/repaper.c
>> +++ b/drivers/gpu/drm/tinydrm/repaper.c
>> @@ -18,6 +18,7 @@
>> */
>> #include <linux/delay.h>
>> +#include <linux/dma-buf.h>
>> #include <linux/gpio/consumer.h>
>> #include <linux/module.h>
>> #include <linux/of_device.h>
>> @@ -525,11 +526,20 @@ static int repaper_fb_dirty(struct
>> drm_framebuffer *fb,
>> struct drm_clip_rect *clips,
>> unsigned int num_clips)
>> {
>> + struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>> + struct dma_buf_attachment *import_attach =
>> cma_obj->base.import_attach;
>> struct tinydrm_device *tdev = fb->dev->dev_private;
>> struct repaper_epd *epd = epd_from_tinydrm(tdev);
>> + struct drm_clip_rect clip;
>> u8 *buf = NULL;
>> int ret = 0;
>> + /* repaper can't do partial updates */
>> + clip.x1 = 0;
>> + clip.x2 = fb->width;
>> + clip.y1 = 0;
>> + clip.y2 = fb->height;
>> +
>> mutex_lock(&tdev->dirty_lock);
>> if (!epd->enabled)
>> @@ -550,7 +560,16 @@ static int repaper_fb_dirty(struct
>> drm_framebuffer *fb,
>> goto out_unlock;
>> }
>> - ret = tinydrm_xrgb8888_to_gray8(buf, fb);
>> + if (import_attach) {
>> + ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
>> + DMA_FROM_DEVICE);
>> + if (ret)
>> + goto out_unlock;
>> + }
>> +
>> + ret = tinydrm_xrgb8888_to_gray8(buf, cma_obj->vaddr, fb, &clip);
>> + if (import_attach)
>> + dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
>
> I think we can make tinydrm_xrgb8888_to_gray8() return void like the
> other copy functions. We loose the error if the line buffer can't be
> allocated, but that's no big deal. If we can't allocate <4k of memory,
> then we have much bigger problems elsewhere.
> This simplifies returning an error from dma_buf_end_cpu_access().
> The error is propagated to userspace who called the dirty ioctl.
>
> When I have switched from cma to shmem buffers backing the framebuffer
> (ongoing work), we don't need the line buffer copy to speed up the
> uncached reads from write combined memory mappings. But we will
> probably need it on buffers imported with PRIME, since we don't know
> the memory mapping. At least I don't think we can find that out.
>

I realised that we can just do a slow copy, if we can't allocate a buffer.
I will sort that out in all the copy functions when we switch to shmem.
So please make tinydrm_xrgb8888_to_gray8() void.

> Noralf.
>
>> if (ret)
>> goto out_unlock;
>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h
>> b/include/drm/tinydrm/tinydrm-helpers.h
>> index a6c387f..9e13ef5 100644
>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> @@ -43,7 +43,8 @@ void tinydrm_swab16(u16 *dst, void *vaddr, struct
>> drm_framebuffer *fb,
>> void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>> struct drm_framebuffer *fb,
>> struct drm_clip_rect *clip, bool swap);
>> -int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb);
>> +int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct
>> drm_framebuffer *fb,
>> + struct drm_clip_rect *clip);
>> struct backlight_device *tinydrm_of_find_backlight(struct device
>> *dev);
>> int tinydrm_enable_backlight(struct backlight_device *backlight);
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>