* [PATCH v3 0/2] drm: automatic legacy gamma support @ 2020-12-10 14:08 Tomi Valkeinen 2020-12-10 14:08 ` [PATCH v3 1/2] " Tomi Valkeinen 2020-12-10 14:08 ` [PATCH v3 2/2] drm: add legacy support for using degamma for gamma Tomi Valkeinen 0 siblings, 2 replies; 8+ messages in thread From: Tomi Valkeinen @ 2020-12-10 14:08 UTC (permalink / raw) To: dri-devel, Daniel Vetter, Ville Syrjälä, Laurent Pinchart Cc: Yannick Fertre, Philippe Cornu, David Airlie, Russell King, Sandy Huang, Paul Cercueil, Tomi Valkeinen, Thomas Zimmermann, Matthias Brugger, Vincent Abriou, Maxime Coquelin, Alexandre Torgue Hi, Another try. I dropped the has_gamma_prop and has_degamma_prop variables and use drm_mode_obj_find_prop_id() instead. I also changed the order of the patches, and added a new helper for setting the gamma ramp to the properties. Tomi Tomi Valkeinen (2): drm: automatic legacy gamma support drm: add legacy support for using degamma for gamma .../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 | 150 ++++++++++++++++-- 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, 160 insertions(+), 121 deletions(-) -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] drm: automatic legacy gamma support 2020-12-10 14:08 [PATCH v3 0/2] drm: automatic legacy gamma support Tomi Valkeinen @ 2020-12-10 14:08 ` Tomi Valkeinen 2020-12-10 15:27 ` Daniel Vetter 2020-12-10 14:08 ` [PATCH v3 2/2] drm: add legacy support for using degamma for gamma Tomi Valkeinen 1 sibling, 1 reply; 8+ messages in thread From: Tomi Valkeinen @ 2020-12-10 14:08 UTC (permalink / raw) To: dri-devel, Daniel Vetter, Ville Syrjälä, Laurent Pinchart Cc: Yannick Fertre, Philippe Cornu, David Airlie, Russell King, Sandy Huang, Paul Cercueil, Tomi Valkeinen, Thomas Zimmermann, Matthias Brugger, Vincent Abriou, Maxime Coquelin, Alexandre Torgue 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" #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); } /** @@ -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; } @@ -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) 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] drm: automatic legacy gamma support 2020-12-10 14:08 ` [PATCH v3 1/2] " Tomi Valkeinen @ 2020-12-10 15:27 ` Daniel Vetter 2020-12-10 15:43 ` Tomi Valkeinen 2020-12-10 16:32 ` Ville Syrjälä 0 siblings, 2 replies; 8+ messages in thread From: Daniel Vetter @ 2020-12-10 15:27 UTC (permalink / raw) To: Tomi Valkeinen Cc: Yannick Fertre, Thomas Zimmermann, Philippe Cornu, David Airlie, Maxime Coquelin, Russell King, dri-devel, Sandy Huang, Paul Cercueil, Alexandre Torgue, Laurent Pinchart, Matthias Brugger, Vincent Abriou 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] drm: automatic legacy gamma support 2020-12-10 15:27 ` Daniel Vetter @ 2020-12-10 15:43 ` Tomi Valkeinen 2020-12-10 16:39 ` Daniel Vetter 2020-12-10 16:32 ` Ville Syrjälä 1 sibling, 1 reply; 8+ messages in thread From: Tomi Valkeinen @ 2020-12-10 15:43 UTC (permalink / raw) To: Daniel Vetter Cc: Yannick Fertre, Philippe Cornu, David Airlie, Maxime Coquelin, Russell King, dri-devel, Sandy Huang, Paul Cercueil, Laurent Pinchart, Thomas Zimmermann, Matthias Brugger, Vincent Abriou, Alexandre Torgue 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. 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. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] drm: automatic legacy gamma support 2020-12-10 15:43 ` Tomi Valkeinen @ 2020-12-10 16:39 ` Daniel Vetter 0 siblings, 0 replies; 8+ messages in thread From: Daniel Vetter @ 2020-12-10 16:39 UTC (permalink / raw) To: Tomi Valkeinen Cc: Yannick Fertre, Philippe Cornu, David Airlie, Maxime Coquelin, Russell King, dri-devel, Sandy Huang, Paul Cercueil, Laurent Pinchart, Thomas Zimmermann, Matthias Brugger, Vincent Abriou, Alexandre Torgue 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] drm: automatic legacy gamma support 2020-12-10 15:27 ` Daniel Vetter 2020-12-10 15:43 ` Tomi Valkeinen @ 2020-12-10 16:32 ` Ville Syrjälä 2020-12-10 16:45 ` Tomi Valkeinen 1 sibling, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2020-12-10 16:32 UTC (permalink / raw) To: Daniel Vetter Cc: Yannick Fertre, Philippe Cornu, David Airlie, Russell King, dri-devel, Sandy Huang, Paul Cercueil, Tomi Valkeinen, Laurent Pinchart, Thomas Zimmermann, Matthias Brugger, Vincent Abriou, Maxime Coquelin, Alexandre Torgue On Thu, Dec 10, 2020 at 04:27:39PM +0100, Daniel Vetter wrote: > 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. 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 ;) -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] drm: automatic legacy gamma support 2020-12-10 16:32 ` Ville Syrjälä @ 2020-12-10 16:45 ` Tomi Valkeinen 0 siblings, 0 replies; 8+ messages in thread From: Tomi Valkeinen @ 2020-12-10 16:45 UTC (permalink / raw) To: Ville Syrjälä, Daniel Vetter Cc: Yannick Fertre, Philippe Cornu, David Airlie, dri-devel, Russell King, Sandy Huang, Paul Cercueil, Laurent Pinchart, Thomas Zimmermann, Matthias Brugger, Vincent Abriou, Maxime Coquelin, Alexandre Torgue 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 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] drm: add legacy support for using degamma for gamma 2020-12-10 14:08 [PATCH v3 0/2] drm: automatic legacy gamma support Tomi Valkeinen 2020-12-10 14:08 ` [PATCH v3 1/2] " Tomi Valkeinen @ 2020-12-10 14:08 ` Tomi Valkeinen 1 sibling, 0 replies; 8+ messages in thread From: Tomi Valkeinen @ 2020-12-10 14:08 UTC (permalink / raw) To: dri-devel, Daniel Vetter, Ville Syrjälä, Laurent Pinchart Cc: Yannick Fertre, Philippe Cornu, David Airlie, Russell King, Sandy Huang, Paul Cercueil, Tomi Valkeinen, Thomas Zimmermann, Matthias Brugger, Vincent Abriou, Maxime Coquelin, Alexandre Torgue The DRM core handles legacy gamma-set ioctl by setting GAMMA_LUT and clearing CTM and DEGAMMA_LUT. This works fine on HW where we have either: degamma -> ctm -> gamma -> out or ctm -> gamma -> out However, if the HW has gamma table before ctm, the atomic property should be DEGAMMA_LUT, and thus we have: degamma -> ctm -> out This is fine for userspace which sets gamma table using the properties, as the userspace can check for the existence of gamma & degamma, but the legacy gamma-set ioctl does not work. Change the DRM core to use DEGAMMA_LUT instead of GAMMA_LUT when the latter is unavailable. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/drm_color_mgmt.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index c4e4d59c4432..8733d9d95b82 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -238,6 +238,7 @@ EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size); bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc) { uint32_t gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id; + uint32_t degamma_id = crtc->dev->mode_config.degamma_lut_property->base.id; if (!crtc->gamma_size) return false; @@ -245,7 +246,8 @@ bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc) if (crtc->funcs->gamma_set) return true; - return !!drm_mode_obj_find_prop_id(&crtc->base, gamma_id); + return !!(drm_mode_obj_find_prop_id(&crtc->base, gamma_id) || + drm_mode_obj_find_prop_id(&crtc->base, degamma_id)); } EXPORT_SYMBOL(drm_crtc_supports_legacy_gamma); @@ -256,18 +258,28 @@ EXPORT_SYMBOL(drm_crtc_supports_legacy_gamma); * @blob: property blob for the gamma ramp * * Set given gamma ramp to the crtc using GAMMA_LUT property and resetting - * DEGAMMA_LUT and CTM. + * DEGAMMA_LUT and CTM, or if GAMMA_LUT is not available, using DEGAMMA_LUT + * and resetting GAMMA_LUT and CTM. */ int drm_crtc_gamma_ramp_set(struct drm_atomic_state *state, struct drm_crtc *crtc, struct drm_property_blob *blob) { + uint32_t gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id; + uint32_t degamma_id = crtc->dev->mode_config.degamma_lut_property->base.id; 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; + if (drm_mode_obj_find_prop_id(&crtc->base, gamma_id)) { + gamma_blob = blob; + degamma_blob = NULL; + } else if (drm_mode_obj_find_prop_id(&crtc->base, degamma_id)) { + gamma_blob = NULL; + degamma_blob = blob; + } else { + return -ENODEV; + } crtc_state = drm_atomic_get_crtc_state(state, crtc); if (IS_ERR(crtc_state)) -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-12-10 16:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-10 14:08 [PATCH v3 0/2] drm: automatic legacy gamma support Tomi Valkeinen 2020-12-10 14:08 ` [PATCH v3 1/2] " Tomi Valkeinen 2020-12-10 15:27 ` Daniel Vetter 2020-12-10 15:43 ` Tomi Valkeinen 2020-12-10 16:39 ` Daniel Vetter 2020-12-10 16:32 ` Ville Syrjälä 2020-12-10 16:45 ` Tomi Valkeinen 2020-12-10 14:08 ` [PATCH v3 2/2] drm: add legacy support for using degamma for gamma Tomi Valkeinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.