[PATCH v2 3/4] drm/tinydrm: Add tinydrm_xrgb8888_to_gray8() helper
Noralf Trønnes
noralf at tronnes.org
Tue Jul 4 15:00:46 UTC 2017
Den 28.06.2017 19.12, skrev Daniel Vetter:
> On Wed, Jun 28, 2017 at 04:26:23PM +0200, Noralf Trønnes wrote:
>> Den 26.06.2017 11.43, skrev Daniel Vetter:
>>> On Thu, Jun 08, 2017 at 05:14:34PM +0200, Noralf Trønnes wrote:
>>>> Drm has no monochrome or greyscale support so add a conversion
>>>> from the common format XR24.
>>>>
>>>> Also reorder includes into the common order.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>>>> ---
>>>> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 74 +++++++++++++++++++++++++-
>>>> include/drm/tinydrm/tinydrm-helpers.h | 1 +
>>>> 2 files changed, 73 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>> index d4cda33..75808bb 100644
>>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>> @@ -7,13 +7,15 @@
>>>> * (at your option) any later version.
>>>> */
>>>> -#include <drm/tinydrm/tinydrm.h>
>>>> -#include <drm/tinydrm/tinydrm-helpers.h>
>>>> #include <linux/backlight.h>
>>>> +#include <linux/dma-buf.h>
>>>> #include <linux/pm.h>
>>>> #include <linux/spi/spi.h>
>>>> #include <linux/swab.h>
>>>> +#include <drm/tinydrm/tinydrm.h>
>>>> +#include <drm/tinydrm/tinydrm-helpers.h>
>>>> +
>>>> static unsigned int spi_max;
>>>> module_param(spi_max, uint, 0400);
>>>> MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
>>>> @@ -181,6 +183,74 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>>>> EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>>>> /**
>>>> + * tinydrm_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
>>>> + * @dst: 8-bit grayscale destination buffer
>>>> + * @fb: DRM framebuffer
>>>> + *
>>>> + * Drm doesn't have native monochrome or grayscale support.
>>>> + * Such drivers can announce the commonly supported XR24 format to userspace
>>>> + * and use this function to convert to the native format.
>>>> + *
>>>> + * Monochrome drivers will use the most significant bit,
>>>> + * where 1 means foreground color and 0 background color.
>>>> + *
>>>> + * ITU BT.601 is used for the RGB -> luma (brightness) conversion.
>>>> + *
>>>> + * Returns:
>>>> + * Zero on success, negative error code on failure.
>>>> + */
>>>> +int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
>>> I think it's ok to use this for backwards compat reasons with userspace
>>> that simply expects xrgb8888, but adding monochrome fourcc codes to drm
>>> isn't hard, and I think would be better to do that here.
>>>
>>> There's also my comment from earlier that too much of tinydrm is in
>>> tinydrm, and maybe we should move some of it out (we already have
>>> tinydrm_xrgb8888_to_rgb565). But looks ok otherwise.
>> I'm not sure that tinydrm_xrgb8888_to_rgb565 and tinydrm_xrgb8888_to_gray8
>> does fit in the core as-is, because they work around the fact that
>> cma memory has uncached reads by buffering one pixel line at a time.
>>
>> I have been aware that the cma library isn't really a prefect match for
>> tinydrm because of this (but it was easy to use). For tiny displays this
>> isn't really a performance problem to speak of, but I see now that it
>> limits which gpus that I can import buffers from, since cma requires
>> continous memory. This is probably the reason the Grain Media driver
>> couldn't use tinydrm or that it needed that shmem thing.
> Hm, might be good to port tinydrm to plain gem shmem objects? We have a
> bunch of gem shmem based simple drivers know, so if helpers aren't as
> comprehensive as with cma that can be fixed (and I'm trying other driver
> submitters to volunteer for that already).
>
>> I'll see if I can use gem/prime code from vgem coupled with prime import
>> code from udl. The spi core claims to do dma transfers on vmalloc
>> addreses, so it seems possible.
> Still feeling guilty for volunteering you for everything, but would be
> awesome if you can make it happen. I think there's other drivers in-flight
> (or even merged) which could benefit from parts of this at least.
I'm volunteering myself this time :-)
Much better to write a library that smart people will improve upon,
than to put something in tinydrm that nobody looks at.
A couple of questions:
- Can the pages be vmap'ed at object creation time, or should this be
an explicit function call? I see some drivers being explicit, but
tinydrm will always need that virtual address.
- Several drivers have cache flags on the gem object used when mmap'ing,
like udl here:
static void update_vm_cache_attr(struct udl_gem_object *obj,
struct vm_area_struct *vma)
{
/* non-cacheable as default. */
if (obj->flags & UDL_BO_CACHEABLE) {
vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
} else if (obj->flags & UDL_BO_WC) {
vma->vm_page_prot =
pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
} else {
vma->vm_page_prot =
pgprot_noncached(vm_get_page_prot(vma->vm_flags));
}
}
Should I do this in the gem shmem library?
drm_driver->gem_create_object can be used by the driver to set the flag.
- And I assume it's better to create a new library instead of
extending the CMA library? Even though most of the code will be the same.
Like this: drm_gem_shmem_helper.{c,h} and drm_fb_shmem_helper.{c,h}
Noralf.
> -Daniel
>
>>
>> Noralf.
>>
>>
>>> Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>
>>> Daniel
>>>
>>>
>>>> +{
>>>> + 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;
>>>> + void *buf;
>>>> + u32 *src;
>>>> +
>>>> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888))
>>>> + return -EINVAL;
>>>> + /*
>>>> + * The cma memory is write-combined so reads are uncached.
>>>> + * Speed up by fetching one line at a time.
>>>> + */
>>>> + buf = kmalloc(pitch, 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);
>>>> + src = buf;
>>>> + for (x = 0; x < fb->width; x++) {
>>>> + u8 r = (*src & 0x00ff0000) >> 16;
>>>> + u8 g = (*src & 0x0000ff00) >> 8;
>>>> + u8 b = *src & 0x000000ff;
>>>> +
>>>> + /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
>>>> + *dst++ = (3 * r + 6 * g + b) / 10;
>>>> + src++;
>>>> + }
>>>> + }
>>>> +
>>>> + if (import_attach)
>>>> + ret = dma_buf_end_cpu_access(import_attach->dmabuf,
>>>> + DMA_FROM_DEVICE);
>>>> +err_free:
>>>> + kfree(buf);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>>>> +
>>>> +/**
>>>> * tinydrm_of_find_backlight - Find backlight device in device-tree
>>>> * @dev: Device
>>>> *
>>>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>>>> index 9b9b6cf..a6c387f 100644
>>>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>>>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>>>> @@ -43,6 +43,7 @@ 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);
>>>> struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
>>>> int tinydrm_enable_backlight(struct backlight_device *backlight);
>>>> --
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list