Date: Mon, 19 Jun 2017 22:09:29 +0200
From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Peter Rosin <peda@axentia.se>
Cc: linux-kernel@vger.kernel.org, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel.vetter@intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Sean Paul <seanpaul@chromium.org>, dri-devel@lists.freedesktop.org,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Alexandre Belloni <alexandre.belloni@free-electrons.com>
Subject: Re: [PATCH v3 0/3] drm: atmel-hlcdc: clut support
Message-ID: <20170619220929.7f88052e@bbrezillon>
In-Reply-To: <1497858266-17844-1-git-send-email-peda@axentia.se>
References: <1497858266-17844-1-git-send-email-peda@axentia.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1558
Lines: 44

Le Mon, 19 Jun 2017 09:44:23 +0200,
Peter Rosin <peda@axentia.se> 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
>