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

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

[Peter Rosin]

Hi!

This series adds support for an 8-bit clut mode in the atmel-hlcdc
driver.

For various reasons I have only tested it with the fbdev interface,
and am therefore not really sure if it works w/o patch 2 and 3.

Also, when using it with the fbdev emulation layer, something seems
to allocate the first 16 clut entries, which throws my users of
ioctl(fb, FBIOPUTCMAP, ...) when index 0 in the given clut is
really pixel value 16. If anyone can shed some light on that, I'd
be most greatful!

Further, I suspect I might need to lock the crtc state when accessing
it in patch 3/3?

Cheers,
peda

Peter Rosin (3):
  atmel-hlcdc: add support for 8-bit color lookup table mode
  drm/fb-cma-helper: expose more of fb cma guts
  atmel-hlcdc: add clut support for legacy fbdev

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  | 97 +++++++++++++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 25 ++++++-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 20 +++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  5 ++
 drivers/gpu/drm/drm_fb_cma_helper.c             | 55 +++++++++++---
 include/drm/drm_fb_cma_helper.h                 |  8 +-
 6 files changed, 197 insertions(+), 13 deletions(-)

-- 
2.1.4

[RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Peter Rosin]

All layers of chips support this, the only variable is the base address
of the lookup table in the register map.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  | 48 +++++++++++++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 13 +++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 16 +++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  5 +++
 4 files changed, 82 insertions(+)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 5348985..75871b5 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -61,6 +61,7 @@ struct atmel_hlcdc_crtc {
 	struct atmel_hlcdc_dc *dc;
 	struct drm_pending_vblank_event *event;
 	int id;
+	u32 clut[ATMEL_HLCDC_CLUT_SIZE];
 };
 
 static inline struct atmel_hlcdc_crtc *
@@ -140,6 +141,46 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 			   cfg);
 }
 
+static void
+atmel_hlcdc_crtc_load_lut(struct drm_crtc *c)
+{
+	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
+	struct atmel_hlcdc_dc *dc = crtc->dc;
+	int layer;
+	int idx;
+
+	for (layer = 0; layer < ATMEL_HLCDC_MAX_LAYERS; layer++) {
+		if (!dc->layers[layer])
+			continue;
+		for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++)
+			atmel_hlcdc_layer_write_clut(dc->layers[layer],
+						     idx, crtc->clut[idx]);
+	}
+}
+
+static void
+atmel_hlcdc_crtc_flush_lut(struct drm_crtc *c)
+{
+	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
+	struct drm_crtc_state *state = c->state;
+	struct drm_color_lut *lut;
+	int idx;
+
+	if (!state->gamma_lut)
+		return;
+
+	lut = (struct drm_color_lut *)state->gamma_lut->data;
+
+	for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) {
+		crtc->clut[idx] =
+			((lut[idx].red << 8) & 0xff0000) |
+			(lut[idx].green & 0xff00) |
+			(lut[idx].blue >> 8);
+	}
+
+	atmel_hlcdc_crtc_load_lut(c);
+}
+
 static enum drm_mode_status
 atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
 			    const struct drm_display_mode *mode)
@@ -312,6 +353,9 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
 					  struct drm_crtc_state *old_s)
 {
 	/* TODO: write common plane control register if available */
+
+	if (crtc->state->color_mgmt_changed)
+		atmel_hlcdc_crtc_flush_lut(crtc);
 }
 
 static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
@@ -429,6 +473,7 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
 	.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
 	.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
 	.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
+	.set_property = drm_atomic_helper_crtc_set_property,
 };
 
 int atmel_hlcdc_crtc_create(struct drm_device *dev)
@@ -484,6 +529,9 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev)
 	drm_crtc_helper_add(&crtc->base, &lcdc_crtc_helper_funcs);
 	drm_crtc_vblank_reset(&crtc->base);
 
+	drm_mode_crtc_set_gamma_size(&crtc->base, ATMEL_HLCDC_CLUT_SIZE);
+	drm_crtc_enable_color_mgmt(&crtc->base, 0, false, ATMEL_HLCDC_CLUT_SIZE);
+
 	dc->crtc = &crtc->base;
 
 	return 0;
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 30dbffd..4f6ef07 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -42,6 +42,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9n12_layers[] = {
 			.default_color = 3,
 			.general_config = 4,
 		},
+		.clut_offset = 0x400,
 	},
 };
 
@@ -73,6 +74,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = {
 			.disc_pos = 5,
 			.disc_size = 6,
 		},
+		.clut_offset = 0x400,
 	},
 	{
 		.name = "overlay1",
@@ -91,6 +93,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = {
 			.chroma_key_mask = 8,
 			.general_config = 9,
 		},
+		.clut_offset = 0x800,
 	},
 	{
 		.name = "high-end-overlay",
@@ -112,6 +115,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = {
 			.scaler_config = 13,
 			.csc = 14,
 		},
+		.clut_offset = 0x1000,
 	},
 	{
 		.name = "cursor",
@@ -131,6 +135,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9x5_layers[] = {
 			.chroma_key_mask = 8,
 			.general_config = 9,
 		},
+		.clut_offset = 0x1400,
 	},
 };
 
@@ -162,6 +167,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = {
 			.disc_pos = 5,
 			.disc_size = 6,
 		},
+		.clut_offset = 0x600,
 	},
 	{
 		.name = "overlay1",
@@ -180,6 +186,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = {
 			.chroma_key_mask = 8,
 			.general_config = 9,
 		},
+		.clut_offset = 0xa00,
 	},
 	{
 		.name = "overlay2",
@@ -198,6 +205,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = {
 			.chroma_key_mask = 8,
 			.general_config = 9,
 		},
+		.clut_offset = 0xe00,
 	},
 	{
 		.name = "high-end-overlay",
@@ -223,6 +231,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = {
 			},
 			.csc = 14,
 		},
+		.clut_offset = 0x1200,
 	},
 	{
 		.name = "cursor",
@@ -244,6 +253,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d3_layers[] = {
 			.general_config = 9,
 			.scaler_config = 13,
 		},
+		.clut_offset = 0x1600,
 	},
 };
 
@@ -275,6 +285,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d4_layers[] = {
 			.disc_pos = 5,
 			.disc_size = 6,
 		},
+		.clut_offset = 0x600,
 	},
 	{
 		.name = "overlay1",
@@ -293,6 +304,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d4_layers[] = {
 			.chroma_key_mask = 8,
 			.general_config = 9,
 		},
+		.clut_offset = 0xa00,
 	},
 	{
 		.name = "overlay2",
@@ -336,6 +348,7 @@ static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sama5d4_layers[] = {
 			},
 			.csc = 14,
 		},
+		.clut_offset = 0x1200,
 	},
 };
 
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index b0596a8..709f7b9 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -88,6 +88,11 @@
 #define ATMEL_HLCDC_YUV422SWP			BIT(17)
 #define ATMEL_HLCDC_DSCALEOPT			BIT(20)
 
+#define ATMEL_HLCDC_C1_MODE			ATMEL_HLCDC_CLUT_MODE(0)
+#define ATMEL_HLCDC_C2_MODE			ATMEL_HLCDC_CLUT_MODE(1)
+#define ATMEL_HLCDC_C4_MODE			ATMEL_HLCDC_CLUT_MODE(2)
+#define ATMEL_HLCDC_C8_MODE			ATMEL_HLCDC_CLUT_MODE(3)
+
 #define ATMEL_HLCDC_XRGB4444_MODE		ATMEL_HLCDC_RGB_MODE(0)
 #define ATMEL_HLCDC_ARGB4444_MODE		ATMEL_HLCDC_RGB_MODE(1)
 #define ATMEL_HLCDC_RGBA4444_MODE		ATMEL_HLCDC_RGB_MODE(2)
@@ -142,6 +147,8 @@
 #define ATMEL_HLCDC_DMA_CHANNEL_DSCR_DONE	BIT(2)
 #define ATMEL_HLCDC_DMA_CHANNEL_DSCR_OVERRUN	BIT(3)
 
+#define ATMEL_HLCDC_CLUT_SIZE			256
+
 #define ATMEL_HLCDC_MAX_LAYERS			6
 
 /**
@@ -259,6 +266,7 @@ struct atmel_hlcdc_layer_desc {
 	int id;
 	int regs_offset;
 	int cfgs_offset;
+	int clut_offset;
 	struct atmel_hlcdc_formats *formats;
 	struct atmel_hlcdc_layer_cfg_layout layout;
 	int max_width;
@@ -414,6 +422,14 @@ static inline u32 atmel_hlcdc_layer_read_cfg(struct atmel_hlcdc_layer *layer,
 					  (cfgid * sizeof(u32)));
 }
 
+static inline void atmel_hlcdc_layer_write_clut(struct atmel_hlcdc_layer *layer,
+						unsigned int c, u32 val)
+{
+	atmel_hlcdc_layer_write_reg(layer,
+				    layer->desc->clut_offset + c * sizeof(u32),
+				    val);
+}
+
 static inline void atmel_hlcdc_layer_init(struct atmel_hlcdc_layer *layer,
 				const struct atmel_hlcdc_layer_desc *desc,
 				struct regmap *regmap)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 1124200..5537843 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -83,6 +83,7 @@ drm_plane_state_to_atmel_hlcdc_plane_state(struct drm_plane_state *s)
 #define SUBPIXEL_MASK			0xffff
 
 static uint32_t rgb_formats[] = {
+	DRM_FORMAT_C8,
 	DRM_FORMAT_XRGB4444,
 	DRM_FORMAT_ARGB4444,
 	DRM_FORMAT_RGBA4444,
@@ -100,6 +101,7 @@ struct atmel_hlcdc_formats atmel_hlcdc_plane_rgb_formats = {
 };
 
 static uint32_t rgb_and_yuv_formats[] = {
+	DRM_FORMAT_C8,
 	DRM_FORMAT_XRGB4444,
 	DRM_FORMAT_ARGB4444,
 	DRM_FORMAT_RGBA4444,
@@ -128,6 +130,9 @@ struct atmel_hlcdc_formats atmel_hlcdc_plane_rgb_and_yuv_formats = {
 static int atmel_hlcdc_format_to_plane_mode(u32 format, u32 *mode)
 {
 	switch (format) {
+	case DRM_FORMAT_C8:
+		*mode = ATMEL_HLCDC_C8_MODE;
+		break;
 	case DRM_FORMAT_XRGB4444:
 		*mode = ATMEL_HLCDC_XRGB4444_MODE;
 		break;
-- 
2.1.4

[RFC PATCH 2/3] drm/fb-cma-helper: expose more of fb cma guts

[Peter Rosin]

DRM drivers supporting clut may want a convenient way to only use
non-default .gamma_set and .gamma_get ops in the drm_fb_helper_funcs
in order to avoid the following

	/*
	 * The driver really shouldn't advertise pseudo/directcolor
	 * visuals if it can't deal with the palette.
	 */
	if (WARN_ON(!fb_helper->funcs->gamma_set ||
		    !fb_helper->funcs->gamma_get))
		return -EINVAL;

warning in drm_fb_helper.c:setcolreg().

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_fb_cma_helper.c | 55 ++++++++++++++++++++++++++++++-------
 include/drm/drm_fb_cma_helper.h     |  8 +++++-
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 53f9bdf..ef96227 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -426,7 +426,12 @@ static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
 	kfree(fbi->fbops);
 }
 
-static int
+/**
+ * drm_fbdev_cma_create() - Default fb_probe() function for fb_cma_helper_funcs
+ * @helper: The fb_helper to create a cma for
+ * @sizes: The fbdev sizes
+ */
+int
 drm_fbdev_cma_create(struct drm_fb_helper *helper,
 	struct drm_fb_helper_surface_size *sizes)
 {
@@ -507,23 +512,28 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper,
 	drm_gem_object_put_unlocked(&obj->base);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(drm_fbdev_cma_create);
 
 static const struct drm_fb_helper_funcs drm_fb_cma_helper_funcs = {
 	.fb_probe = drm_fbdev_cma_create,
 };
 
 /**
- * drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct
+ * drm_fbdev_cma_init_with_funcs2() - Allocate and initializes a drm_fbdev_cma struct
  * @dev: DRM device
  * @preferred_bpp: Preferred bits per pixel for the device
  * @max_conn_count: Maximum number of connectors
- * @funcs: fb helper functions, in particular a custom dirty() callback
+ * @framebuffer_funcs: framebuffer functions, in particular a custom dirty() callback
+ * @fb_helper_funcs: fb helper functions, in particular custom gamma_set() and gamma_get() callbacks
+ *
+ * If framebuffer_funcs or fb_helper_funcs are NULL, default functions are used.
  *
  * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
  */
-struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
+struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs2(struct drm_device *dev,
 	unsigned int preferred_bpp, unsigned int max_conn_count,
-	const struct drm_framebuffer_funcs *funcs)
+	const struct drm_framebuffer_funcs *framebuffer_funcs,
+	const struct drm_fb_helper_funcs *fb_helper_funcs)
 {
 	struct drm_fbdev_cma *fbdev_cma;
 	struct drm_fb_helper *helper;
@@ -534,11 +544,17 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
 		dev_err(dev->dev, "Failed to allocate drm fbdev.\n");
 		return ERR_PTR(-ENOMEM);
 	}
-	fbdev_cma->fb_funcs = funcs;
+
+	if (!framebuffer_funcs)
+		framebuffer_funcs = &drm_fb_cma_funcs;
+	if (!fb_helper_funcs)
+		fb_helper_funcs = &drm_fb_cma_helper_funcs;
+
+	fbdev_cma->fb_funcs = framebuffer_funcs;
 
 	helper = &fbdev_cma->fb_helper;
 
-	drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
+	drm_fb_helper_prepare(dev, helper, fb_helper_funcs);
 
 	ret = drm_fb_helper_init(dev, helper, max_conn_count);
 	if (ret < 0) {
@@ -568,6 +584,25 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
 
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs2);
+
+/**
+ * drm_fbdev_cma_init_with_funcs() - Allocate and initializes a drm_fbdev_cma struct
+ * @dev: DRM device
+ * @preferred_bpp: Preferred bits per pixel for the device
+ * @max_conn_count: Maximum number of connectors
+ * @framebuffer_funcs: framebuffer functions, in particular a custom dirty() callback
+ *
+ * Returns a newly allocated drm_fbdev_cma struct or a ERR_PTR.
+ */
+struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
+	unsigned int preferred_bpp, unsigned int max_conn_count,
+	const struct drm_framebuffer_funcs *framebuffer_funcs)
+{
+	return drm_fbdev_cma_init_with_funcs2(dev, preferred_bpp,
+					      max_conn_count,
+					      framebuffer_funcs, NULL);
+}
 EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);
 
 /**
@@ -581,9 +616,9 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);
 struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 	unsigned int preferred_bpp, unsigned int max_conn_count)
 {
-	return drm_fbdev_cma_init_with_funcs(dev, preferred_bpp,
-					     max_conn_count,
-					     &drm_fb_cma_funcs);
+	return drm_fbdev_cma_init_with_funcs2(dev, preferred_bpp,
+					      max_conn_count,
+					      NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
 
diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
index 199a63f..280ec2b 100644
--- a/include/drm/drm_fb_cma_helper.h
+++ b/include/drm/drm_fb_cma_helper.h
@@ -15,13 +15,19 @@ struct drm_mode_fb_cmd2;
 struct drm_plane;
 struct drm_plane_state;
 
+struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs2(struct drm_device *dev,
+	unsigned int preferred_bpp, unsigned int max_conn_count,
+	const struct drm_framebuffer_funcs *framebuffer_funcs,
+	const struct drm_fb_helper_funcs *fb_helper_funcs);
 struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
 	unsigned int preferred_bpp, unsigned int max_conn_count,
-	const struct drm_framebuffer_funcs *funcs);
+	const struct drm_framebuffer_funcs *framebuffer_funcs);
 struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
 	unsigned int preferred_bpp, unsigned int max_conn_count);
 void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma);
 
+int drm_fbdev_cma_create(struct drm_fb_helper *helper,
+	struct drm_fb_helper_surface_size *sizes);
 void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma);
 void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
 void drm_fbdev_cma_set_suspend(struct drm_fbdev_cma *fbdev_cma, int state);
-- 
2.1.4

[RFC PATCH 3/3] atmel-hlcdc: add clut support for legacy fbdev

[Peter Rosin]

The clut is also synchronized with the drm gamma_lut state.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 49 ++++++++++++++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c   | 12 +++++--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  4 +++
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 75871b5..54ecf56 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -158,6 +158,54 @@ atmel_hlcdc_crtc_load_lut(struct drm_crtc *c)
 	}
 }
 
+void atmel_hlcdc_gamma_set(struct drm_crtc *c,
+			   u16 r, u16 g, u16 b, int idx)
+{
+	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
+	struct drm_crtc_state *state = c->state;
+	struct drm_color_lut *lut;
+
+	if (idx < 0 || idx > 255)
+		return;
+
+	if (state->gamma_lut) {
+		lut = (struct drm_color_lut *)state->gamma_lut->data;
+
+		lut[idx].red = r;
+		lut[idx].green = g;
+		lut[idx].blue = b;
+	}
+
+	crtc->clut[idx] = ((r << 8) & 0xff0000) | (g & 0xff00) | (b >> 8);
+}
+
+void atmel_hlcdc_gamma_get(struct drm_crtc *c,
+			   u16 *r, u16 *g, u16 *b, int idx)
+{
+	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
+	struct drm_crtc_state *state = c->state;
+	struct drm_color_lut *lut;
+
+	if (idx < 0 || idx > 255)
+		return;
+
+	if (state->gamma_lut) {
+		lut = (struct drm_color_lut *)state->gamma_lut->data;
+
+		*r = lut[idx].red;
+		*g = lut[idx].green;
+		*b = lut[idx].blue;
+		return;
+	}
+
+	*r = (crtc->clut[idx] >> 8) & 0xff00;
+	*g = crtc->clut[idx] & 0xff00;
+	*b = (crtc->clut[idx] << 8) & 0xff00;
+	*r |= *r >> 8;
+	*g |= *g >> 8;
+	*b |= *b >> 8;
+}
+
 static void
 atmel_hlcdc_crtc_flush_lut(struct drm_crtc *c)
 {
@@ -363,6 +411,7 @@ static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
 	.mode_set = drm_helper_crtc_mode_set,
 	.mode_set_nofb = atmel_hlcdc_crtc_mode_set_nofb,
 	.mode_set_base = drm_helper_crtc_mode_set_base,
+	.load_lut = atmel_hlcdc_crtc_load_lut,
 	.disable = atmel_hlcdc_crtc_disable,
 	.enable = atmel_hlcdc_crtc_enable,
 	.atomic_check = atmel_hlcdc_crtc_atomic_check,
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 4f6ef07..9a09c73 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -601,6 +601,12 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
 	return 0;
 }
 
+static const struct drm_fb_helper_funcs atmel_hlcdc_fb_cma_helper_funcs = {
+	.gamma_set	= atmel_hlcdc_gamma_set,
+	.gamma_get	= atmel_hlcdc_gamma_get,
+	.fb_probe	= drm_fbdev_cma_create,
+};
+
 static int atmel_hlcdc_dc_load(struct drm_device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev->dev);
@@ -664,8 +670,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 
 	platform_set_drvdata(pdev, dev);
 
-	dc->fbdev = drm_fbdev_cma_init(dev, 24,
-			dev->mode_config.num_connector);
+	dc->fbdev = drm_fbdev_cma_init_with_funcs2(dev, 24,
+			dev->mode_config.num_connector,
+			NULL,
+			&atmel_hlcdc_fb_cma_helper_funcs);
 	if (IS_ERR(dc->fbdev))
 		dc->fbdev = NULL;
 
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index 709f7b9..1b13224 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -32,6 +32,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_panel.h>
@@ -448,6 +449,9 @@ void atmel_hlcdc_plane_irq(struct atmel_hlcdc_plane *plane);
 int atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state *c_state);
 int atmel_hlcdc_plane_prepare_ahb_routing(struct drm_crtc_state *c_state);
 
+void atmel_hlcdc_gamma_set(struct drm_crtc *c, u16 r, u16 g, u16 b, int idx);
+void atmel_hlcdc_gamma_get(struct drm_crtc *c, u16 *r, u16 *g, u16 *b, int idx);
+
 void atmel_hlcdc_crtc_irq(struct drm_crtc *c);
 
 int atmel_hlcdc_crtc_create(struct drm_device *dev);
-- 
2.1.4

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Boris Brezillon]

Hi Peter,

On Fri, 16 Jun 2017 11:12:25 +0200
Peter Rosin <peda@axentia.se> wrote:

> All layers of chips support this, the only variable is the base address
> of the lookup table in the register map.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  | 48 +++++++++++++++++++++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 13 +++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 16 +++++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  5 +++
>  4 files changed, 82 insertions(+)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 5348985..75871b5 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -61,6 +61,7 @@ struct atmel_hlcdc_crtc {
>  	struct atmel_hlcdc_dc *dc;
>  	struct drm_pending_vblank_event *event;
>  	int id;
> +	u32 clut[ATMEL_HLCDC_CLUT_SIZE];

Do we really need to duplicate this table here? I mean, the gamma_lut
table should always be available in the crtc_state, so do you have a
good reason a copy here?

>  };
>  
>  static inline struct atmel_hlcdc_crtc *
> @@ -140,6 +141,46 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  			   cfg);
>  }
>  
> +static void
> +atmel_hlcdc_crtc_load_lut(struct drm_crtc *c)
> +{
> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> +	struct atmel_hlcdc_dc *dc = crtc->dc;
> +	int layer;
> +	int idx;
> +
> +	for (layer = 0; layer < ATMEL_HLCDC_MAX_LAYERS; layer++) {
> +		if (!dc->layers[layer])
> +			continue;
> +		for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++)
> +			atmel_hlcdc_layer_write_clut(dc->layers[layer],
> +						     idx, crtc->clut[idx]);
> +	}
> +}
> +
> +static void
> +atmel_hlcdc_crtc_flush_lut(struct drm_crtc *c)
> +{
> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> +	struct drm_crtc_state *state = c->state;
> +	struct drm_color_lut *lut;
> +	int idx;
> +
> +	if (!state->gamma_lut)
> +		return;
> +
> +	lut = (struct drm_color_lut *)state->gamma_lut->data;
> +
> +	for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) {
> +		crtc->clut[idx] =
> +			((lut[idx].red << 8) & 0xff0000) |
> +			(lut[idx].green & 0xff00) |
> +			(lut[idx].blue >> 8);
> +	}
> +
> +	atmel_hlcdc_crtc_load_lut(c);
> +}
> +
>  static enum drm_mode_status
>  atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
>  			    const struct drm_display_mode *mode)
> @@ -312,6 +353,9 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
>  					  struct drm_crtc_state *old_s)
>  {
>  	/* TODO: write common plane control register if available */
> +
> +	if (crtc->state->color_mgmt_changed)
> +		atmel_hlcdc_crtc_flush_lut(crtc);

Hm, it's probably too late to do it here. Planes have already been
enabled and the engine may have started to fetch data and do the
composition. You could do that in ->update_plane() [1], and make it a
per-plane thing.

I'm not sure, but I think you can get the new crtc_state from
plane->crtc->state in this context (state have already been swapped,
and new state is being applied, which means relevant locks are held).


>  }
>  
>  static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
> @@ -429,6 +473,7 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
>  	.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
>  	.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
>  	.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
> +	.set_property = drm_atomic_helper_crtc_set_property,

Well, this change is independent from gamma LUT support. Should
probably be done in a separate patch.


Also, you should probably have:
 
	.gamma_set = drm_atomic_helper_legacy_gamma_set,

>  };

The rest looks good.

Thanks,

Boris

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Peter Rosin]

On 2017-06-16 12:01, Boris Brezillon wrote:
> Hi Peter,
> 
> On Fri, 16 Jun 2017 11:12:25 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> All layers of chips support this, the only variable is the base address
>> of the lookup table in the register map.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  | 48 +++++++++++++++++++++++++
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 13 +++++++
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 16 +++++++++
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  5 +++
>>  4 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> index 5348985..75871b5 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> @@ -61,6 +61,7 @@ struct atmel_hlcdc_crtc {
>>  	struct atmel_hlcdc_dc *dc;
>>  	struct drm_pending_vblank_event *event;
>>  	int id;
>> +	u32 clut[ATMEL_HLCDC_CLUT_SIZE];
> 
> Do we really need to duplicate this table here? I mean, the gamma_lut
> table should always be available in the crtc_state, so do you have a
> good reason a copy here?

If I don't keep a copy in the driver, it doesn't work when there's no
gamma_lut. And there is no gamma_lut when I use fbdev emulation. Maybe
that's a bug somewhere else?

Sure, I could have added it in patch 3/3 instead, but didn't when I
divided the work into patches...

>>  };
>>  
>>  static inline struct atmel_hlcdc_crtc *
>> @@ -140,6 +141,46 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>  			   cfg);
>>  }
>>  
>> +static void
>> +atmel_hlcdc_crtc_load_lut(struct drm_crtc *c)
>> +{
>> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
>> +	struct atmel_hlcdc_dc *dc = crtc->dc;
>> +	int layer;
>> +	int idx;
>> +
>> +	for (layer = 0; layer < ATMEL_HLCDC_MAX_LAYERS; layer++) {
>> +		if (!dc->layers[layer])
>> +			continue;
>> +		for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++)
>> +			atmel_hlcdc_layer_write_clut(dc->layers[layer],
>> +						     idx, crtc->clut[idx]);
>> +	}
>> +}
>> +
>> +static void
>> +atmel_hlcdc_crtc_flush_lut(struct drm_crtc *c)
>> +{
>> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
>> +	struct drm_crtc_state *state = c->state;
>> +	struct drm_color_lut *lut;
>> +	int idx;
>> +
>> +	if (!state->gamma_lut)
>> +		return;
>> +
>> +	lut = (struct drm_color_lut *)state->gamma_lut->data;
>> +
>> +	for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) {
>> +		crtc->clut[idx] =
>> +			((lut[idx].red << 8) & 0xff0000) |
>> +			(lut[idx].green & 0xff00) |
>> +			(lut[idx].blue >> 8);
>> +	}
>> +
>> +	atmel_hlcdc_crtc_load_lut(c);
>> +}
>> +
>>  static enum drm_mode_status
>>  atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
>>  			    const struct drm_display_mode *mode)
>> @@ -312,6 +353,9 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
>>  					  struct drm_crtc_state *old_s)
>>  {
>>  	/* TODO: write common plane control register if available */
>> +
>> +	if (crtc->state->color_mgmt_changed)
>> +		atmel_hlcdc_crtc_flush_lut(crtc);
> 
> Hm, it's probably too late to do it here. Planes have already been
> enabled and the engine may have started to fetch data and do the
> composition. You could do that in ->update_plane() [1], and make it a
> per-plane thing.
> 
> I'm not sure, but I think you can get the new crtc_state from
> plane->crtc->state in this context (state have already been swapped,
> and new state is being applied, which means relevant locks are held).

Ok, I can move it there. My plan is to just copy the default .update_plane
function and insert 

	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) {
		...
	}

just before the drm_atomic_commit(state) call. Sounds ok?

>>  }
>>  
>>  static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
>> @@ -429,6 +473,7 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
>>  	.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
>>  	.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
>>  	.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
>> +	.set_property = drm_atomic_helper_crtc_set_property,
> 
> Well, this change is independent from gamma LUT support. Should
> probably be done in a separate patch.

Ok, I think I fat-fingered some kernel cmdline at some point and fooled
myself into thinking I needed it for some reason...

> Also, you should probably have:
>  
> 	.gamma_set = drm_atomic_helper_legacy_gamma_set,

That doesn't no anything for me, but sure, I can add it.

Cheers,
peda

>>  };
> 
> The rest looks good.
> 
> Thanks,
> 
> Boris
> 

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Boris Brezillon]

Hi Peter,

On Fri, 16 Jun 2017 17:54:04 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2017-06-16 12:01, Boris Brezillon wrote:
> > Hi Peter,
> > 
> > On Fri, 16 Jun 2017 11:12:25 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> All layers of chips support this, the only variable is the base address
> >> of the lookup table in the register map.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  | 48 +++++++++++++++++++++++++
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 13 +++++++
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 16 +++++++++
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  5 +++
> >>  4 files changed, 82 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> >> index 5348985..75871b5 100644
> >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> >> @@ -61,6 +61,7 @@ struct atmel_hlcdc_crtc {
> >>  	struct atmel_hlcdc_dc *dc;
> >>  	struct drm_pending_vblank_event *event;
> >>  	int id;
> >> +	u32 clut[ATMEL_HLCDC_CLUT_SIZE];  
> > 
> > Do we really need to duplicate this table here? I mean, the gamma_lut
> > table should always be available in the crtc_state, so do you have a
> > good reason a copy here?  
> 
> If I don't keep a copy in the driver, it doesn't work when there's no
> gamma_lut. And there is no gamma_lut when I use fbdev emulation. Maybe
> that's a bug somewhere else?

Can't we re-use crtc->gamma_store? Honnestly, I don't know how the
fbdev->DRM link should be done, so we'd better wait for DRM maintainers
feedback here (Daniel, any opinion?).

> 
> Sure, I could have added it in patch 3/3 instead, but didn't when I
> divided the work into patches...

No, my point is that IMO it shouldn't be needed at all.

> 
> >>  };
> >>  
> >>  static inline struct atmel_hlcdc_crtc *
> >> @@ -140,6 +141,46 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
> >>  			   cfg);
> >>  }
> >>  
> >> +static void
> >> +atmel_hlcdc_crtc_load_lut(struct drm_crtc *c)
> >> +{
> >> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> >> +	struct atmel_hlcdc_dc *dc = crtc->dc;
> >> +	int layer;
> >> +	int idx;
> >> +
> >> +	for (layer = 0; layer < ATMEL_HLCDC_MAX_LAYERS; layer++) {
> >> +		if (!dc->layers[layer])
> >> +			continue;
> >> +		for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++)
> >> +			atmel_hlcdc_layer_write_clut(dc->layers[layer],
> >> +						     idx, crtc->clut[idx]);
> >> +	}
> >> +}
> >> +
> >> +static void
> >> +atmel_hlcdc_crtc_flush_lut(struct drm_crtc *c)
> >> +{
> >> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
> >> +	struct drm_crtc_state *state = c->state;
> >> +	struct drm_color_lut *lut;
> >> +	int idx;
> >> +
> >> +	if (!state->gamma_lut)
> >> +		return;
> >> +
> >> +	lut = (struct drm_color_lut *)state->gamma_lut->data;
> >> +
> >> +	for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) {
> >> +		crtc->clut[idx] =
> >> +			((lut[idx].red << 8) & 0xff0000) |
> >> +			(lut[idx].green & 0xff00) |
> >> +			(lut[idx].blue >> 8);
> >> +	}
> >> +
> >> +	atmel_hlcdc_crtc_load_lut(c);
> >> +}
> >> +
> >>  static enum drm_mode_status
> >>  atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
> >>  			    const struct drm_display_mode *mode)
> >> @@ -312,6 +353,9 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
> >>  					  struct drm_crtc_state *old_s)
> >>  {
> >>  	/* TODO: write common plane control register if available */
> >> +
> >> +	if (crtc->state->color_mgmt_changed)
> >> +		atmel_hlcdc_crtc_flush_lut(crtc);  
> > 
> > Hm, it's probably too late to do it here. Planes have already been
> > enabled and the engine may have started to fetch data and do the
> > composition. You could do that in ->update_plane() [1], and make it a
> > per-plane thing.
> > 
> > I'm not sure, but I think you can get the new crtc_state from
> > plane->crtc->state in this context (state have already been swapped,
> > and new state is being applied, which means relevant locks are held).  
> 
> Ok, I can move it there. My plan is to just copy the default .update_plane
> function and insert 
> 
> 	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) {
> 		...
> 	}
> 
> just before the drm_atomic_commit(state) call. Sounds ok?

Why would you copy the default ->update_plane() when we already have
our own ->atomic_update_plane() implementation [1]? Just put it there
(before the atmel_hlcdc_layer_update_commit() call) and we should be
good.

> 
> >>  }
> >>  
> >>  static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
> >> @@ -429,6 +473,7 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
> >>  	.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
> >>  	.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
> >>  	.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
> >> +	.set_property = drm_atomic_helper_crtc_set_property,  
> > 
> > Well, this change is independent from gamma LUT support. Should
> > probably be done in a separate patch.  
> 
> Ok, I think I fat-fingered some kernel cmdline at some point and fooled
> myself into thinking I needed it for some reason...

It's probably a good thing to have it set anyway.

> 
> > Also, you should probably have:
> >  
> > 	.gamma_set = drm_atomic_helper_legacy_gamma_set,  
> 
> That doesn't no anything for me, but sure, I can add it.

To be very clear, I'd like you to test it through DRM ioctls, not only
through the fbdev emulation layer.

Regards,

Boris

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Peter Rosin]

On 2017-06-16 18:15, Boris Brezillon wrote:
> Hi Peter,
> 
> On Fri, 16 Jun 2017 17:54:04 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> On 2017-06-16 12:01, Boris Brezillon wrote:
>>> Hi Peter,
>>>
>>> On Fri, 16 Jun 2017 11:12:25 +0200
>>> Peter Rosin <peda@axentia.se> wrote:
>>>   
>>>> All layers of chips support this, the only variable is the base address
>>>> of the lookup table in the register map.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  | 48 +++++++++++++++++++++++++
>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 13 +++++++
>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 16 +++++++++
>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  5 +++
>>>>  4 files changed, 82 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>>> index 5348985..75871b5 100644
>>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>>>> @@ -61,6 +61,7 @@ struct atmel_hlcdc_crtc {
>>>>  	struct atmel_hlcdc_dc *dc;
>>>>  	struct drm_pending_vblank_event *event;
>>>>  	int id;
>>>> +	u32 clut[ATMEL_HLCDC_CLUT_SIZE];  
>>>
>>> Do we really need to duplicate this table here? I mean, the gamma_lut
>>> table should always be available in the crtc_state, so do you have a
>>> good reason a copy here?  
>>
>> If I don't keep a copy in the driver, it doesn't work when there's no
>> gamma_lut. And there is no gamma_lut when I use fbdev emulation. Maybe
>> that's a bug somewhere else?
> 
> Can't we re-use crtc->gamma_store? Honnestly, I don't know how the
> fbdev->DRM link should be done, so we'd better wait for DRM maintainers
> feedback here (Daniel, any opinion?).

Ahh, gamma_store. Makes perfect sense. Thanks for that pointer!

>>
>> Sure, I could have added it in patch 3/3 instead, but didn't when I
>> divided the work into patches...
> 
> No, my point is that IMO it shouldn't be needed at all.

Right, with gamma_store it's no longer needed.

>>
>>>>  };
>>>>  
>>>>  static inline struct atmel_hlcdc_crtc *
>>>> @@ -140,6 +141,46 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>>>  			   cfg);
>>>>  }
>>>>  
>>>> +static void
>>>> +atmel_hlcdc_crtc_load_lut(struct drm_crtc *c)
>>>> +{
>>>> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
>>>> +	struct atmel_hlcdc_dc *dc = crtc->dc;
>>>> +	int layer;
>>>> +	int idx;
>>>> +
>>>> +	for (layer = 0; layer < ATMEL_HLCDC_MAX_LAYERS; layer++) {
>>>> +		if (!dc->layers[layer])
>>>> +			continue;
>>>> +		for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++)
>>>> +			atmel_hlcdc_layer_write_clut(dc->layers[layer],
>>>> +						     idx, crtc->clut[idx]);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void
>>>> +atmel_hlcdc_crtc_flush_lut(struct drm_crtc *c)
>>>> +{
>>>> +	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
>>>> +	struct drm_crtc_state *state = c->state;
>>>> +	struct drm_color_lut *lut;
>>>> +	int idx;
>>>> +
>>>> +	if (!state->gamma_lut)
>>>> +		return;
>>>> +
>>>> +	lut = (struct drm_color_lut *)state->gamma_lut->data;
>>>> +
>>>> +	for (idx = 0; idx < ATMEL_HLCDC_CLUT_SIZE; idx++) {
>>>> +		crtc->clut[idx] =
>>>> +			((lut[idx].red << 8) & 0xff0000) |
>>>> +			(lut[idx].green & 0xff00) |
>>>> +			(lut[idx].blue >> 8);
>>>> +	}
>>>> +
>>>> +	atmel_hlcdc_crtc_load_lut(c);
>>>> +}
>>>> +
>>>>  static enum drm_mode_status
>>>>  atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
>>>>  			    const struct drm_display_mode *mode)
>>>> @@ -312,6 +353,9 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
>>>>  					  struct drm_crtc_state *old_s)
>>>>  {
>>>>  	/* TODO: write common plane control register if available */
>>>> +
>>>> +	if (crtc->state->color_mgmt_changed)
>>>> +		atmel_hlcdc_crtc_flush_lut(crtc);  
>>>
>>> Hm, it's probably too late to do it here. Planes have already been
>>> enabled and the engine may have started to fetch data and do the
>>> composition. You could do that in ->update_plane() [1], and make it a
>>> per-plane thing.
>>>
>>> I'm not sure, but I think you can get the new crtc_state from
>>> plane->crtc->state in this context (state have already been swapped,
>>> and new state is being applied, which means relevant locks are held).  
>>
>> Ok, I can move it there. My plan is to just copy the default .update_plane
>> function and insert 
>>
>> 	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) {
>> 		...
>> 	}
>>
>> just before the drm_atomic_commit(state) call. Sounds ok?
> 
> Why would you copy the default ->update_plane() when we already have
> our own ->atomic_update_plane() implementation [1]? Just put it there
> (before the atmel_hlcdc_layer_update_commit() call) and we should be
> good.

Ahh, but you said ->update_plane() and I took that as .update_plane in
layer_plane_funcs, not ->atomic_update() in atmel_hlcdc_layer_plane_helper_funcs.

Makes sense now, and much neater too.

>>
>>>>  }
>>>>  
>>>>  static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
>>>> @@ -429,6 +473,7 @@ static const struct drm_crtc_funcs atmel_hlcdc_crtc_funcs = {
>>>>  	.atomic_destroy_state = atmel_hlcdc_crtc_destroy_state,
>>>>  	.enable_vblank = atmel_hlcdc_crtc_enable_vblank,
>>>>  	.disable_vblank = atmel_hlcdc_crtc_disable_vblank,
>>>> +	.set_property = drm_atomic_helper_crtc_set_property,  
>>>
>>> Well, this change is independent from gamma LUT support. Should
>>> probably be done in a separate patch.  
>>
>> Ok, I think I fat-fingered some kernel cmdline at some point and fooled
>> myself into thinking I needed it for some reason...
> 
> It's probably a good thing to have it set anyway.

Looking at the code, I think it's needed since I think that's how the
gamma_lut property is modified for the non-emulated case...

>>
>>> Also, you should probably have:
>>>  
>>> 	.gamma_set = drm_atomic_helper_legacy_gamma_set,  
>>
>> That doesn't no anything for me, but sure, I can add it.
> 
> To be very clear, I'd like you to test it through DRM ioctls, not only
> through the fbdev emulation layer.

...so yeah, right, I couldn't agree more. Any pointers to code w/o a bunch
of complex library dependencies that I can test with?

Cheers,
peda

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Peter Rosin]

On 2017-06-16 23:12, Peter Rosin wrote:
> On 2017-06-16 18:15, Boris Brezillon wrote:
>> To be very clear, I'd like you to test it through DRM ioctls, not only
>> through the fbdev emulation layer.
> 
> ...so yeah, right, I couldn't agree more. Any pointers to code w/o a bunch
> of complex library dependencies that I can test with?

I have now built libdrm-2.4.81, and get this:

$ modetest -M atmel-hlcdc -s 27@39:1024x768
setting mode 1024x768-60Hz@XR24 on connectors 27, crtc 39
$ modetest -M atmel-hlcdc -s 27@39:1024x768@RG16
setting mode 1024x768-60Hz@RG16 on connectors 27, crtc 39
$ modetest -M atmel-hlcdc -s 27@39:1024x768@C8
unknown format C8
<usage>

(output on the lcd looks sane for the first two, not that I really
know exactly what to expect)

Looking at the libdrm code, I only find YUV and RGB modes in
tests/util/format.c which make me less confident that I will find
something sane to test with.

So, pointers to code to test with desperately needed...

Cheers,
peda

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Peter Rosin]

>>>> Hm, it's probably too late to do it here. Planes have already been
>>>> enabled and the engine may have started to fetch data and do the
>>>> composition. You could do that in ->update_plane() [1], and make it a
>>>> per-plane thing.
>>>>
>>>> I'm not sure, but I think you can get the new crtc_state from
>>>> plane->crtc->state in this context (state have already been swapped,
>>>> and new state is being applied, which means relevant locks are held).  
>>>
>>> Ok, I can move it there. My plan is to just copy the default .update_plane
>>> function and insert 
>>>
>>> 	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) {
>>> 		...
>>> 	}
>>>
>>> just before the drm_atomic_commit(state) call. Sounds ok?
>>
>> Why would you copy the default ->update_plane() when we already have
>> our own ->atomic_update_plane() implementation [1]? Just put it there
>> (before the atmel_hlcdc_layer_update_commit() call) and we should be
>> good.
> 
> Ahh, but you said ->update_plane() and I took that as .update_plane in
> layer_plane_funcs, not ->atomic_update() in atmel_hlcdc_layer_plane_helper_funcs.
> 
> Makes sense now, and much neater too.

No, it doesn't make sense. There's no atmel_hlcdc_layer_update_commit call
anywhere, and no such function. You seem to have some further changes that
are not even in -next. Where am I getting those changes and why are they
not upstream yet?

There's a mention of the missing function here [1], but that's some 18
months ago. What's going on?

[1] https://patchwork.kernel.org/patch/7965721/

Cheers,
peda

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Boris Brezillon]

Le Sat, 17 Jun 2017 00:46:12 +0200,
Peter Rosin <peda@axentia.se> a écrit :

> >>>> Hm, it's probably too late to do it here. Planes have already been
> >>>> enabled and the engine may have started to fetch data and do the
> >>>> composition. You could do that in ->update_plane() [1], and make it a
> >>>> per-plane thing.
> >>>>
> >>>> I'm not sure, but I think you can get the new crtc_state from
> >>>> plane->crtc->state in this context (state have already been swapped,
> >>>> and new state is being applied, which means relevant locks are held).    
> >>>
> >>> Ok, I can move it there. My plan is to just copy the default .update_plane
> >>> function and insert 
> >>>
> >>> 	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) {
> >>> 		...
> >>> 	}
> >>>
> >>> just before the drm_atomic_commit(state) call. Sounds ok?  
> >>
> >> Why would you copy the default ->update_plane() when we already have
> >> our own ->atomic_update_plane() implementation [1]? Just put it there
> >> (before the atmel_hlcdc_layer_update_commit() call) and we should be
> >> good.  
> > 
> > Ahh, but you said ->update_plane() and I took that as .update_plane in
> > layer_plane_funcs, not ->atomic_update() in atmel_hlcdc_layer_plane_helper_funcs.
> > 
> > Makes sense now, and much neater too.  
> 
> No, it doesn't make sense. There's no atmel_hlcdc_layer_update_commit call
> anywhere, and no such function. You seem to have some further changes that
> are not even in -next. Where am I getting those changes and why are they
> not upstream yet?

My bad, this part as been reworked in 4.12 and I was reading 4.11 code.
Indeed, atmel_hlcdc_layer_update_commit() no longer exists, but
atmel_hlcdc_plane_atomic_update() does.

Just add a function called atmel_hlcdc_plane_update_clut() in
atmel_hlcdc_plane.c and call it just after [1].

[1]http://elixir.free-electrons.com/linux/v4.12-rc5/source/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c#L770

Re: [RFC PATCH 1/3] atmel-hlcdc: add support for 8-bit color lookup table mode

[Peter Rosin]

On 2017-06-17 07:36, Boris Brezillon wrote:
> Le Sat, 17 Jun 2017 00:46:12 +0200,
> Peter Rosin <peda@axentia.se> a écrit :
> 
>>>>>> Hm, it's probably too late to do it here. Planes have already been
>>>>>> enabled and the engine may have started to fetch data and do the
>>>>>> composition. You could do that in ->update_plane() [1], and make it a
>>>>>> per-plane thing.
>>>>>>
>>>>>> I'm not sure, but I think you can get the new crtc_state from
>>>>>> plane->crtc->state in this context (state have already been swapped,
>>>>>> and new state is being applied, which means relevant locks are held).    
>>>>>
>>>>> Ok, I can move it there. My plan is to just copy the default .update_plane
>>>>> function and insert 
>>>>>
>>>>> 	if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut) {
>>>>> 		...
>>>>> 	}
>>>>>
>>>>> just before the drm_atomic_commit(state) call. Sounds ok?  
>>>>
>>>> Why would you copy the default ->update_plane() when we already have
>>>> our own ->atomic_update_plane() implementation [1]? Just put it there
>>>> (before the atmel_hlcdc_layer_update_commit() call) and we should be
>>>> good.  
>>>
>>> Ahh, but you said ->update_plane() and I took that as .update_plane in
>>> layer_plane_funcs, not ->atomic_update() in atmel_hlcdc_layer_plane_helper_funcs.
>>>
>>> Makes sense now, and much neater too.  
>>
>> No, it doesn't make sense. There's no atmel_hlcdc_layer_update_commit call
>> anywhere, and no such function. You seem to have some further changes that
>> are not even in -next. Where am I getting those changes and why are they
>> not upstream yet?
> 
> My bad, this part as been reworked in 4.12 and I was reading 4.11 code.
> Indeed, atmel_hlcdc_layer_update_commit() no longer exists, but
> atmel_hlcdc_plane_atomic_update() does.

Ah, it was the other way around. I had too new code!

Anyway, I just sent a new series which I think addresses all issues except
that I have still not tested with plain drm ioctls.

Cheers,
peda