On Thu, Dec 10, 2020 at 04:08:51PM +0200, Tomi Valkeinen wrote:

To support legacy gamma ioctls the drivers need to set drm_crtc_funcs.gamma_set either to a custom implementation or to drm_atomic_helper_legacy_gamma_set. Most of the atomic drivers do the latter.

We can simplify this by making the core handle it automatically.

Add three functions to drm_color_mgmt.c:

drm_crtc_supports_legacy_gamma() which tells if the driver supports setting the legacy gamma.

drm_crtc_gamma_ramp_set() which sets the given gamma ramp to GAMMA_LUT and resets DEGAMMA_LUT and CTM.

drm_crtc_legacy_gamma_set() which sets the given gamma ramp values either using drm_crtc_funcs.gamma_set or drm_crtc_gamma_ramp_set().

These functions are used from the drm_mode_gamma_set_ioctl, and from drm_fb_helper.c when it is dealing with fbdev cmap.

We can then drop drm_atomic_helper_legacy_gamma_set() and remove all its uses.

Note that we need to EXPORT_SYMBOL all the new functions as they are used from drm_fb_helper, but they are declared in drm_crtc_internal.h as they are not supposed to be used by the drivers.

Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 - .../gpu/drm/arm/display/komeda/komeda_crtc.c | 1 - drivers/gpu/drm/arm/malidp_crtc.c | 1 - drivers/gpu/drm/armada/armada_crtc.c | 1 - drivers/gpu/drm/ast/ast_mode.c | 1 - .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 1 - drivers/gpu/drm/drm_atomic_helper.c | 70 --------- drivers/gpu/drm/drm_color_mgmt.c | 138 ++++++++++++++++-- drivers/gpu/drm/drm_crtc_internal.h | 10 ++ drivers/gpu/drm/drm_fb_helper.c | 28 ++-- drivers/gpu/drm/i915/display/intel_display.c | 1 - drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 - drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 - drivers/gpu/drm/nouveau/dispnv50/head.c | 2 - drivers/gpu/drm/omapdrm/omap_crtc.c | 1 - drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 1 - drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 1 - drivers/gpu/drm/stm/ltdc.c | 1 - drivers/gpu/drm/vc4/vc4_crtc.c | 1 - drivers/gpu/drm/vc4/vc4_txp.c | 1 - include/drm/drm_atomic_helper.h | 4 - include/drm/drm_color_mgmt.h | 1 - 22 files changed, 148 insertions(+), 121 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 2855bb918535..848b06c51b0e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5396,7 +5396,6 @@ static void dm_disable_vblank(struct drm_crtc *crtc) static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = { .reset = dm_crtc_reset_state, .destroy = amdgpu_dm_crtc_destroy,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, .atomic_duplicate_state = dm_crtc_duplicate_state,

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c index 4b485eb512e2..59172acb9738 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c @@ -550,7 +550,6 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc) }

static const struct drm_crtc_funcs komeda_crtc_funcs = {

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip,

diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c index 108e7a31bd26..494075ddbef6 100644 --- a/drivers/gpu/drm/arm/malidp_crtc.c +++ b/drivers/gpu/drm/arm/malidp_crtc.c @@ -510,7 +510,6 @@ static void malidp_crtc_disable_vblank(struct drm_crtc *crtc) }

static const struct drm_crtc_funcs malidp_crtc_funcs = {

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip,

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index 3ebcf5a52c8b..b7bb90ae787f 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -820,7 +820,6 @@ static const struct drm_crtc_funcs armada_crtc_funcs = { .cursor_set = armada_drm_crtc_cursor_set, .cursor_move = armada_drm_crtc_cursor_move, .destroy = armada_drm_crtc_destroy,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 9db371f4054f..5b0ec785c516 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -903,7 +903,6 @@ static void ast_crtc_atomic_destroy_state(struct drm_crtc *crtc,

static const struct drm_crtc_funcs ast_crtc_funcs = { .reset = ast_crtc_reset,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip,

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index c58fa00b4848..05ad75d155e8 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -473,7 +473,6 @@ 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,

• .gamma_set = drm_atomic_helper_legacy_gamma_set,

};

int atmel_hlcdc_crtc_create(struct drm_device *dev) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..b114658100b3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3499,76 +3499,6 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);

-/**

• • drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table
• • @crtc: CRTC object
• • @red: red correction table
• • @green: green correction table
• • @blue: green correction table
• • @size: size of the tables
• • @ctx: lock acquire context
• • Implements support for legacy gamma correction table for drivers
• • that support color management through the DEGAMMA_LUT/GAMMA_LUT
• • properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
• • how the atomic color management and gamma tables work.
• */

-int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,

• 		       u16 *red, u16 *green, u16 *blue,
  

• 		       uint32_t size,
  

• 		       struct drm_modeset_acquire_ctx *ctx)
  

-{

• struct drm_device *dev = crtc->dev;
• struct drm_atomic_state *state;
• struct drm_crtc_state *crtc_state;
• struct drm_property_blob *blob = NULL;
• struct drm_color_lut *blob_data;
• int i, ret = 0;
• bool replaced;
• state = drm_atomic_state_alloc(crtc->dev);
• if (!state)

• return -ENOMEM;
  

• blob = drm_property_create_blob(dev,

• 			sizeof(struct drm_color_lut) * size,
  

• 			NULL);
  

• if (IS_ERR(blob)) {

• ret = PTR_ERR(blob);
  

• blob = NULL;
  

• goto fail;
  

• }
• /* Prepare GAMMA_LUT with the legacy values. */
• blob_data = blob->data;
• for (i = 0; i < size; i++) {

• blob_data[i].red = red[i];
  

• blob_data[i].green = green[i];
  

• blob_data[i].blue = blue[i];
  

• }
• state->acquire_ctx = ctx;
• crtc_state = drm_atomic_get_crtc_state(state, crtc);
• if (IS_ERR(crtc_state)) {

• ret = PTR_ERR(crtc_state);
  

• goto fail;
  

• }
• /* Reset DEGAMMA_LUT and CTM properties. */
• replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
• replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
• replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
• crtc_state->color_mgmt_changed |= replaced;
• ret = drm_atomic_commit(state);

-fail:

• drm_atomic_state_put(state);
• drm_property_blob_put(blob);
• return ret;

-} -EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);

/**

• drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to

• 				  the input end of a bridge
  

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..c4e4d59c4432 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -22,6 +22,7 @@

#include <linux/uaccess.h>

+#include <drm/drm_atomic.h> #include <drm/drm_color_mgmt.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> @@ -89,9 +90,8 @@

• modes) appropriately.
• There is also support for a legacy gamma table, which is set up by calling

• • drm_mode_crtc_set_gamma_size(). Drivers which support both should use
• • drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
• • "GAMMA_LUT" property above.

• • drm_mode_crtc_set_gamma_size(). The DRM core will then alias the legacy gamma
• • ramp with "GAMMA_LUT".
  • Support for different non RGB color encodings is controlled through
  • &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They

@@ -156,9 +156,6 @@ EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);

• optional. The gamma and degamma properties are only attached if
• their size is not 0 and ctm_property is only attached if has_ctm is
• true.

• • Drivers should use drm_atomic_helper_legacy_gamma_set() to implement the
• • legacy &drm_crtc_funcs.gamma_set callback.
  */

void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, uint degamma_lut_size, @@ -231,6 +228,129 @@ int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size);

+/**

• • drm_crtc_supports_legacy_gamma - does the crtc support legacy gamma correction table
• • @crtc: CRTC object
• • Returns true/false if the given crtc supports setting the legacy gamma
• • correction table.
• */

+bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc) +{

• uint32_t gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
• if (!crtc->gamma_size)

• return false;
  

• if (crtc->funcs->gamma_set)

• return true;
  

• return !!drm_mode_obj_find_prop_id(&crtc->base, gamma_id);

+} +EXPORT_SYMBOL(drm_crtc_supports_legacy_gamma);

+/**

• • drm_crtc_gamma_ramp_set - set gamma ramp using GAMMA_LUT property
• • @state: atomic state to which the gamma ramp is set
• • @crtc: CRTC object
• • @blob: property blob for the gamma ramp
• • Set given gamma ramp to the crtc using GAMMA_LUT property and resetting
• • DEGAMMA_LUT and CTM.
• */

+int drm_crtc_gamma_ramp_set(struct drm_atomic_state *state, struct drm_crtc *crtc,

•        struct drm_property_blob *blob)
  

+{

• struct drm_crtc_state *crtc_state;
• struct drm_property_blob *gamma_blob;
• struct drm_property_blob *degamma_blob;
• bool replaced;
• gamma_blob = blob;
• degamma_blob = NULL;
• crtc_state = drm_atomic_get_crtc_state(state, crtc);
• if (IS_ERR(crtc_state))

• return PTR_ERR(crtc_state);
  

• /* Set GAMMA_LUT and reset DEGAMMA_LUT and CTM */
• replaced = drm_property_replace_blob(&crtc_state->degamma_lut, degamma_blob);
• replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
• replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, gamma_blob);
• crtc_state->color_mgmt_changed |= replaced;
• return 0;

+} +EXPORT_SYMBOL(drm_crtc_gamma_ramp_set);

+/**

• • drm_crtc_legacy_gamma_set - set the legacy gamma correction table
• • @crtc: CRTC object
• • @red: red correction table
• • @green: green correction table
• • @blue: green correction table
• • @size: size of the tables
• • @ctx: lock acquire context
• • Implements support for legacy gamma correction table for drivers
• • that have set drm_crtc_funcs.gamma_set or that support color management
• • through the DEGAMMA_LUT/GAMMA_LUT properties. See
• • drm_crtc_enable_color_mgmt() and the containing chapter for
• • how the atomic color management and gamma tables work.
• • This function sets the gamma using drm_crtc_funcs.gamma_set if set, or
• • alternatively using crtc color management properties.
• */

+int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,

• 	      u16 *red, u16 *green, u16 *blue,
  

• 	      uint32_t size,
  

• 	      struct drm_modeset_acquire_ctx *ctx)
  

+{

• struct drm_device *dev = crtc->dev;
• struct drm_atomic_state *state;
• struct drm_property_blob *blob;
• struct drm_color_lut *blob_data;
• int i, ret = 0;
• if (crtc->funcs->gamma_set)

• return crtc->funcs->gamma_set(crtc, red, green, blue, size, ctx);
  

• state = drm_atomic_state_alloc(crtc->dev);
• if (!state)

• return -ENOMEM;
  

• blob = drm_property_create_blob(dev,

• 			sizeof(struct drm_color_lut) * size,
  

• 			NULL);
  

• if (IS_ERR(blob)) {

• ret = PTR_ERR(blob);
  

• blob = NULL;
  

• goto fail;
  

• }
• /* Prepare GAMMA_LUT with the legacy values. */
• blob_data = blob->data;
• for (i = 0; i < size; i++) {

• blob_data[i].red = red[i];
  

• blob_data[i].green = green[i];
  

• blob_data[i].blue = blue[i];
  

• }
• state->acquire_ctx = ctx;
• ret = drm_crtc_gamma_ramp_set(state, crtc, blob);
• if (ret)

• goto fail;
  

• ret = drm_atomic_commit(state);

+fail:

• drm_atomic_state_put(state);
• drm_property_blob_put(blob);
• return ret;

+} +EXPORT_SYMBOL(drm_crtc_legacy_gamma_set);

/**

• drm_mode_gamma_set_ioctl - set the gamma table
• @dev: DRM device

@@ -262,7 +382,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev, if (!crtc) return -ENOENT;

• if (crtc->funcs->gamma_set == NULL)

• if (!drm_crtc_supports_legacy_gamma(crtc)) return -ENOSYS;

  /* memcpy into gamma store */

@@ -290,8 +410,8 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev, goto out; }

• ret = crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,

• 		     crtc->gamma_size, &ctx);
  

• ret = drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,

• 			crtc->gamma_size, &ctx);
  
  

out: DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 54d4cf1233e9..74856df6482a 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -51,9 +51,11 @@ struct drm_mode_fb_cmd2; struct drm_mode_fb_cmd; struct drm_mode_object; struct drm_mode_set; +struct drm_modeset_acquire_ctx; struct drm_plane; struct drm_plane_state; struct drm_property; +struct drm_property_blob; struct edid; struct kref; struct work_struct; @@ -114,6 +116,14 @@ int drm_mode_destroy_dumb_ioctl(struct drm_device *dev, const char *drm_get_color_encoding_name(enum drm_color_encoding encoding); const char *drm_get_color_range_name(enum drm_color_range range);

+bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc); +int drm_crtc_gamma_ramp_set(struct drm_atomic_state *state, struct drm_crtc *crtc,

•        struct drm_property_blob *blob);
  

+int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc,

• 	      u16 *red, u16 *green, u16 *blue,
  

• 	      uint32_t size,
  

• 	      struct drm_modeset_acquire_ctx *ctx);
  

/* IOCTLs */ int drm_mode_gamma_get_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index e82db0f4e771..80e3797f0f01 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -46,6 +46,7 @@ #include <drm/drm_print.h> #include <drm/drm_vblank.h>

+#include "drm_crtc_internal.h"

So this is a bit annoying, because thus far we managed to have a very clear split between core and helpers. And I think we can keep that.

#include "drm_crtc_helper_internal.h" #include "drm_internal.h"

@@ -136,15 +137,15 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) { uint16_t *r_base, *g_base, *b_base;

• if (crtc->funcs->gamma_set == NULL)

• if (!drm_crtc_supports_legacy_gamma(crtc)) return;

  r_base = crtc->gamma_store; g_base = r_base + crtc->gamma_size; b_base = g_base + crtc->gamma_size;

• crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,

• 	       crtc->gamma_size, NULL);
  

• drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,

• 		  crtc->gamma_size, NULL);
  

This is only used by legacy non-atomic drivers. It's pretty much impossible to make kgdb work with atomic drivers, so really let's just not bother and keep the code as-is.

}

/** @@ -946,7 +947,7 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) drm_modeset_lock_all(fb_helper->dev); drm_client_for_each_modeset(modeset, &fb_helper->client) { crtc = modeset->crtc;

• if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
  

• if (!drm_crtc_supports_legacy_gamma(crtc)) {
  ret = -EINVAL;
  goto out;
  

  }

@@ -964,8 +965,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));

• ret = crtc->funcs->gamma_set(crtc, r, g, b,
  

• 			     crtc->gamma_size, NULL);
  

• ret = drm_crtc_legacy_gamma_set(crtc, r, g, b, crtc->gamma_size,
  

• 				NULL);
  

  if (ret) goto out;

Same here.

} @@ -1024,12 +1025,10 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) struct drm_device *dev = fb_helper->dev; struct drm_property_blob *gamma_lut = NULL; struct drm_modeset_acquire_ctx ctx;

• struct drm_crtc_state *crtc_state; struct drm_atomic_state *state; struct drm_mode_set *modeset; struct drm_crtc *crtc; u16 *r, *g, *b;

• bool replaced; int ret = 0;

  drm_modeset_acquire_init(&ctx, 0);

@@ -1053,18 +1052,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) goto out_state; }

• crtc_state = drm_atomic_get_crtc_state(state, crtc);
  

• if (IS_ERR(crtc_state)) {
  

• 	ret = PTR_ERR(crtc_state);
  

• ret = drm_crtc_gamma_ramp_set(state, crtc, gamma_lut);
  

• if (ret)
  

You're nesting an atomic commit in an atomic commit here, that will go boom. I guess ideally we'd move this into drm_client_modeset so it remembers the fbdev gamma ramp and does it all in one go. Otherwise I guess you need some kind of different helper, not sure what. -Daniel

	goto out_state;

• }
  

• replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
  

• 				      NULL);
  

• replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
  

• replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
  

• 				      gamma_lut);
  

• crtc_state->color_mgmt_changed |= replaced;
  

  }

  ret = drm_atomic_commit(state);

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index cddbda5303ff..1e90b8c3b1c7 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -16879,7 +16879,6 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, }

#define INTEL_CRTC_FUNCS \

• .gamma_set = drm_atomic_helper_legacy_gamma_set, \ .set_config = drm_atomic_helper_set_config, \ .destroy = intel_crtc_destroy, \ .page_flip = drm_atomic_helper_page_flip, \

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 42d335d3a114..7bb31fbee29d 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -774,8 +774,6 @@ static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {

.enable_vblank = ingenic_drm_enable_vblank, .disable_vblank = ingenic_drm_disable_vblank,

• .gamma_set = drm_atomic_helper_legacy_gamma_set,

};

static const struct drm_plane_helper_funcs ingenic_drm_plane_helper_funcs = { diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index bfe994230543..b665bd498a4a 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -619,7 +619,6 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = { .reset = mtk_drm_crtc_reset, .atomic_duplicate_state = mtk_drm_crtc_duplicate_state, .atomic_destroy_state = mtk_drm_crtc_destroy_state,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .enable_vblank = mtk_drm_crtc_enable_vblank, .disable_vblank = mtk_drm_crtc_disable_vblank,

}; diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c index 537c1ef2e464..ec361d17e900 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/head.c +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c @@ -503,7 +503,6 @@ nv50_head_destroy(struct drm_crtc *crtc) static const struct drm_crtc_funcs nv50_head_func = { .reset = nv50_head_reset,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy = nv50_head_destroy, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip,

@@ -518,7 +517,6 @@ nv50_head_func = { static const struct drm_crtc_funcs nvd9_head_func = { .reset = nv50_head_reset,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy = nv50_head_destroy, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip,

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 7d66269ad998..b97f21cf4a91 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -741,7 +741,6 @@ static const struct drm_crtc_funcs omap_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = omap_crtc_destroy, .page_flip = drm_atomic_helper_page_flip,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .atomic_duplicate_state = omap_crtc_duplicate_state, .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, .atomic_set_property = omap_crtc_atomic_set_property,

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index b5fb941e0f53..f93e0750431d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -1144,7 +1144,6 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = { .set_crc_source = rcar_du_crtc_set_crc_source, .verify_crc_source = rcar_du_crtc_verify_crc_source, .get_crc_sources = rcar_du_crtc_get_crc_sources,

• .gamma_set = drm_atomic_helper_legacy_gamma_set,

};

/* ----------------------------------------------------------------------------- diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index d1e05482641b..8d15cabdcb02 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1643,7 +1643,6 @@ static const struct drm_crtc_funcs vop_crtc_funcs = { .disable_vblank = vop_crtc_disable_vblank, .set_crc_source = vop_crtc_set_crc_source, .verify_crc_source = vop_crtc_verify_crc_source,

• .gamma_set = drm_atomic_helper_legacy_gamma_set,

};

static void vop_fb_unref_worker(struct drm_flip_work *work, void *val) diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 3980677435cb..7812094f93d6 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -713,7 +713,6 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = { .enable_vblank = ltdc_crtc_enable_vblank, .disable_vblank = ltdc_crtc_disable_vblank, .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,

• .gamma_set = drm_atomic_helper_legacy_gamma_set,

};

/* diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index ea710beb8e00..7495d70d6a32 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -884,7 +884,6 @@ static const struct drm_crtc_funcs vc4_crtc_funcs = { .reset = vc4_crtc_reset, .atomic_duplicate_state = vc4_crtc_duplicate_state, .atomic_destroy_state = vc4_crtc_destroy_state,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .enable_vblank = vc4_enable_vblank, .disable_vblank = vc4_disable_vblank, .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 8aa5220885f4..e4b782e42f69 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -382,7 +382,6 @@ static const struct drm_crtc_funcs vc4_txp_crtc_funcs = { .reset = vc4_crtc_reset, .atomic_duplicate_state = vc4_crtc_duplicate_state, .atomic_destroy_state = vc4_crtc_destroy_state,

• .gamma_set = drm_atomic_helper_legacy_gamma_set, .enable_vblank = vc4_txp_enable_vblank, .disable_vblank = vc4_txp_disable_vblank,

}; diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 5f47720440fa..4045e2507e11 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -147,10 +147,6 @@ int drm_atomic_helper_page_flip_target( uint32_t flags, uint32_t target, struct drm_modeset_acquire_ctx *ctx); -int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,

• 		       u16 *red, u16 *green, u16 *blue,
  

• 		       uint32_t size,
  

• 		       struct drm_modeset_acquire_ctx *ctx);
  
  

/**

• drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC

diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h index 81c298488b0c..c41e56e7e7f5 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -61,7 +61,6 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,

int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, int gamma_size);

/**

• drm_color_lut_size - calculate the number of entries in the LUT
• @blob: blob containing the LUT

-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On Thu, Dec 10, 2020 at 4:43 PM Tomi Valkeinen tomi.valkeinen@ti.com wrote:

On 10/12/2020 17:27, Daniel Vetter wrote:

...

...

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index e82db0f4e771..80e3797f0f01 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -46,6 +46,7 @@ #include <drm/drm_print.h> #include <drm/drm_vblank.h>

+#include "drm_crtc_internal.h"

So this is a bit annoying, because thus far we managed to have a very clear split between core and helpers. And I think we can keep that.

...

#include "drm_crtc_helper_internal.h" #include "drm_internal.h"

@@ -136,15 +137,15 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) { uint16_t *r_base, *g_base, *b_base;

• if (crtc->funcs->gamma_set == NULL)

• if (!drm_crtc_supports_legacy_gamma(crtc)) return;

  r_base = crtc->gamma_store; g_base = r_base + crtc->gamma_size; b_base = g_base + crtc->gamma_size;

• crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,

•                       crtc->gamma_size, NULL);
  

• drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,

•                          crtc->gamma_size, NULL);
  

This is only used by legacy non-atomic drivers. It's pretty much impossible to make kgdb work with atomic drivers, so really let's just not bother and keep the code as-is.

You're right.

...

...

}

/** @@ -946,7 +947,7 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) drm_modeset_lock_all(fb_helper->dev); drm_client_for_each_modeset(modeset, &fb_helper->client) { crtc = modeset->crtc;

•        if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
  

•        if (!drm_crtc_supports_legacy_gamma(crtc)) {
                 ret = -EINVAL;
                 goto out;
         }
  

@@ -964,8 +965,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));

•        ret = crtc->funcs->gamma_set(crtc, r, g, b,
  

•                                     crtc->gamma_size, NULL);
  

•        ret = drm_crtc_legacy_gamma_set(crtc, r, g, b, crtc->gamma_size,
  

•                                        NULL);
         if (ret)
                 goto out;
  

Same here.

Yep.

...

...

 }

@@ -1024,12 +1025,10 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) struct drm_device *dev = fb_helper->dev; struct drm_property_blob *gamma_lut = NULL; struct drm_modeset_acquire_ctx ctx;

• struct drm_crtc_state *crtc_state; struct drm_atomic_state *state; struct drm_mode_set *modeset; struct drm_crtc *crtc; u16 *r, *g, *b;

• bool replaced; int ret = 0;

  drm_modeset_acquire_init(&ctx, 0);

@@ -1053,18 +1052,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) goto out_state; }

•        crtc_state = drm_atomic_get_crtc_state(state, crtc);
  

•        if (IS_ERR(crtc_state)) {
  

•                ret = PTR_ERR(crtc_state);
  

•        ret = drm_crtc_gamma_ramp_set(state, crtc, gamma_lut);
  

•        if (ret)
  

You're nesting an atomic commit in an atomic commit here, that will go boom. I guess ideally we'd move this into drm_client_modeset so it remembers the fbdev gamma ramp and does it all in one go. Otherwise I guess you need some kind of different helper, not sure what.

What do you mean? Are you mixing drm_crtc_legacy_gamma_set with drm_crtc_gamma_ramp_set (yeah, I didn't quite know how to name the latter one...)? drm_crtc_gamma_ramp_set does the same thing as the removed code, it sets the gamma_lut in to the state. It doesn't commit.

I mean my brain's not working today at all :-/ You're right.

drm_crtc_gamma_ramp_set does a "setup the state so that this gamma ramp will be on screen", which means setting/clearing GAMMA_LUT, DEGAMMA_LUT and CTM. I wanted to have that logic in one place, which means we need to export it from drm.ko.

I could just inline drm_crtc_gamma_ramp_set, but then I need drm_mode_obj_find_prop_id, which is again not exported. I could also inline drm_mode_obj_find_prop_id as it's trivial enough loop. But this sounds uglier than exporting a function.

Personally, I don't remember when I have used fbdev the last time (other than simple tests), and I could as well just leave the code here as it is. I have no idea if this fbdev setcmap is a big feature that has to function also with HW that only has a pre-gamma table.

I have honestly no idea how much this is used. Maybe just throw a FIXME comment on top of this here that it doesn't handle degamma and should be moved into drm_client_modeset.c? And then leave the code unchanged, we're not breaking anything by not implementing degamma support for fbdev. Whomever cares can address that in a neat and clean fashion. -Daniel

On 10/12/2020 18:32, Ville Syrjälä wrote:

...

...

@@ -1053,18 +1052,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) goto out_state; }

• crtc_state = drm_atomic_get_crtc_state(state, crtc);
  

• if (IS_ERR(crtc_state)) {
  

• 	ret = PTR_ERR(crtc_state);
  

• ret = drm_crtc_gamma_ramp_set(state, crtc, gamma_lut);
  

• if (ret)
  

You're nesting an atomic commit in an atomic commit here, that will go boom. I guess ideally we'd move this into drm_client_modeset so it remembers the fbdev gamma ramp and does it all in one go.

IIRC that's pretty much what's in my branch that I mentioned earlier. Mentioning it again in case someone wants a leg up for implementing this idea ;)

Yep, I think your branch looks good, and I almost started working on it, but I don't think I have the time for it. I have to finish this by early next week, after which I don't know when I could continue with it (if ever, job change...).

Tomi