Re: [PATCH v3 0/3] drm: atmel-hlcdc: clut support

[Boris Brezillon]

Le Mon, 19 Jun 2017 09:44:23 +0200,
Peter Rosin  a écrit :

> Hi!
> 
> This series adds support for an 8-bit clut mode in the atmel-hlcdc
> driver.
> 
> I have now tested patch 1 with the below program (modeset.c
> adapted from https://github.com/dvdhrm/docs/tree/master/drm-howto
> to use an 8-bit mode).

I'm glad you finally find a way to test it. Patch 1 looks good to me,
except I would have added the missing .set_property in a separate patch
(placed at the beginning of the series).

> 
> Since v2 I have also cleared up why the first 16 entries of the clut
> was not working right. It was of course my own damn fault, and the
> fix was in atmel_hlcdc_layer_write_clut function which called the
> ...write_reg function which in turn added an extra offset of 16
> registers...
> 
> Changes since v2:
> 
> - Fix mapping to the clut registers.
> 
> Changes since v1:
> 
> - Move the clut update from atmel_hlcdc_crtc_mode_valid to
>   atmel_hlcdc_plane_atomic_update.
> - Add default .gamma_set helper (drm_atomic_helper_legacy_gamma_set).
> - Don't keep a spare copy of the clut, reuse gamma_store instead.
> - Don't try to synchronize the legacy fb clut with the drm clut.
> 
> As I said in v2, I have not added any .clut_offset to the overlay2
> layer of sama5d4, since the chip does not appear to have that layer.
> I didn't do that to make it easier to work with the patch previously
> sent to remove that layer, but I suspect bad things may happen to
> sama5d4 users if they do not have that layer removed.
> 
> Cheers,
> peda
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/3] drm: atmel-hlcdc: clut support

[Boris Brezillon]

+Alexandre and Nicolas

Hi Peter,

Can you please Cc at91 maintainers next time?

On Mon, 19 Jun 2017 09:44:23 +0200
Peter Rosin  wrote:

> Hi!
> 
> This series adds support for an 8-bit clut mode in the atmel-hlcdc
> driver.
> 
> I have now tested patch 1 with the below program (modeset.c
> adapted from https://github.com/dvdhrm/docs/tree/master/drm-howto
> to use an 8-bit mode).
> 
> Since v2 I have also cleared up why the first 16 entries of the clut
> was not working right. It was of course my own damn fault, and the
> fix was in atmel_hlcdc_layer_write_clut function which called the
> ...write_reg function which in turn added an extra offset of 16
> registers...
> 
> Changes since v2:
> 
> - Fix mapping to the clut registers.
> 
> Changes since v1:
> 
> - Move the clut update from atmel_hlcdc_crtc_mode_valid to
>   atmel_hlcdc_plane_atomic_update.
> - Add default .gamma_set helper (drm_atomic_helper_legacy_gamma_set).
> - Don't keep a spare copy of the clut, reuse gamma_store instead.
> - Don't try to synchronize the legacy fb clut with the drm clut.
> 
> As I said in v2, I have not added any .clut_offset to the overlay2
> layer of sama5d4, since the chip does not appear to have that layer.
> I didn't do that to make it easier to work with the patch previously
> sent to remove that layer, but I suspect bad things may happen to
> sama5d4 users if they do not have that layer removed.
> 
> Cheers,
> peda
> 
> modeset-pal.c (didn't update any comments, sorry)
> 8<---
> /*
>  * modeset - DRM Modesetting Example
>  *
>  * Written 2012 by David Herrmann 
>  * Dedicated to the Public Domain.
>  */
> 
> /*
>  * DRM Modesetting Howto
>  * This document describes the DRM modesetting API. Before we can use the DRM
>  * API, we have to include xf86drm.h and xf86drmMode.h. Both are provided by
>  * libdrm which every major distribution ships by default. It has no other
>  * dependencies and is pretty small.
>  *
>  * Please ignore all forward-declarations of functions which are used later. I
>  * reordered the functions so you can read this document from top to bottom. 
> If
>  * you reimplement it, you would probably reorder the functions to avoid all 
> the
>  * nasty forward declarations.
>  *
>  * For easier reading, we ignore all memory-allocation errors of malloc() and
>  * friends here. However, we try to correctly handle all other kinds of errors
>  * that may occur.
>  *
>  * All functions and global variables are prefixed with "modeset_*" in this
>  * file. So it should be clear whether a function is a local helper or if it 
> is
>  * provided by some external library.
>  */
> 
> #define _GNU_SOURCE
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> 
> struct modeset_dev;
> static int modeset_find_crtc(int fd, drmModeRes *res, drmModeConnector *conn,
>struct modeset_dev *dev);
> static int modeset_create_fb(int fd, struct modeset_dev *dev);
> static int modeset_setup_dev(int fd, drmModeRes *res, drmModeConnector *conn,
>struct modeset_dev *dev);
> static int modeset_open(int *out, const char *node);
> static int modeset_prepare(int fd);
> static void modeset_draw(int fd);
> static void modeset_cleanup(int fd);
> 
> /*
>  * When the linux kernel detects a graphics-card on your machine, it loads the
>  * correct device driver (located in kernel-tree at ./drivers/gpu/drm/) 
> and
>  * provides two character-devices to control it. Udev (or whatever hotplugging
>  * application you use) will create them as:
>  * /dev/dri/card0
>  * /dev/dri/controlID64
>  * We only need the first one. You can hard-code this path into your 
> application
>  * like we do here, but it is recommended to use libudev with real hotplugging
>  * and multi-seat support. However, this is beyond the scope of this document.
>  * Also note that if you have multiple graphics-cards, there may also be
>  * /dev/dri/card1, /dev/dri/card2, ...
>  *
>  * We simply use /dev/dri/card0 here but the user can specify another path on
>  * the command line.
>  *
>  * modeset_open(out, node): This small helper function opens the DRM device
>  * which is given as @node. The new fd is stored in @out on success. On 
> failure,
>  * a negative error code is returned.
>  * After opening the file, we also check for the DRM_CAP_DUMB_BUFFER 
> capability.
>  * If the driver supports this capability, we can create simple memory-mapped
>  * buffers without any driver-dependent code. As we want to avoid any radeon,
>  * nvidia, intel, etc. specific code, we depend on DUMB_BUFFERs here.
>  */
> 
> static int modeset_open(int *out, const char *node)
> {
>   int fd, ret;
>   uint64_t has_dumb;
> 
>   fd = open(node, O_RDWR | O_CLOEXEC);
>   if (fd < 0) {
>   ret = -errno;
>   fprintf(stderr, "cannot