* [PATCH v2] mgag200: Enable atomic gamma lut update
@ 2022-05-11 15:28 Jocelyn Falempe
2022-05-11 21:14 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jocelyn Falempe @ 2022-05-11 15:28 UTC (permalink / raw)
To: dri-devel, lyude, tzimmermann; +Cc: michel, Jocelyn Falempe
Add support for atomic update of gamma lut.
With this patch the "Night light" feature of gnome3
is working properly on mgag200.
v2:
- Add a default linear gamma function
- renamed functions with mgag200 prefix
- use format's 4cc code instead of bit depth
- use better interpolation for 16bits gamma
- remove legacy function mga_crtc_load_lut()
- can't remove the call to drm_mode_crtc_set_gamma_size()
because it doesn't work with userspace.
- other small refactors
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
drivers/gpu/drm/mgag200/mgag200_mode.c | 125 ++++++++++++++++---------
1 file changed, 81 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 6e18d3bbd720..b748bc5b0e93 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -32,57 +32,76 @@
* This file contains setup code for the CRTC.
*/
-static void mga_crtc_load_lut(struct drm_crtc *crtc)
+static void mgag200_crtc_set_gamma_linear(struct mga_device *mdev,
+ uint32_t format)
{
- struct drm_device *dev = crtc->dev;
- struct mga_device *mdev = to_mga_device(dev);
- struct drm_framebuffer *fb;
- u16 *r_ptr, *g_ptr, *b_ptr;
int i;
- if (!crtc->enabled)
- return;
-
- if (!mdev->display_pipe.plane.state)
- return;
+ WREG8(DAC_INDEX + MGA1064_INDEX, 0);
- fb = mdev->display_pipe.plane.state->fb;
+ switch (format) {
+ case DRM_FORMAT_RGB565:
+ /* Use better interpolation, to take 32 values from 0 to 255 */
+ for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4);
+ }
+ /* Green has one more bit, so add padding with 0 for red and blue. */
+ for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
+ }
+ break;
+ case DRM_FORMAT_RGB888:
+ case DRM_FORMAT_XRGB8888:
+ for (i = 0; i < MGAG200_LUT_SIZE; i++) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
+ }
+ break;
+ default:
+ drm_warn_once(&mdev->base, "Unsupported format for gamma %d\n", format);
+ break;
+ }
+}
- r_ptr = crtc->gamma_store;
- g_ptr = r_ptr + crtc->gamma_size;
- b_ptr = g_ptr + crtc->gamma_size;
+static void mgag200_crtc_set_gamma(struct mga_device *mdev,
+ struct drm_color_lut *lut,
+ uint32_t format)
+{
+ int i;
WREG8(DAC_INDEX + MGA1064_INDEX, 0);
- if (fb && fb->format->cpp[0] * 8 == 16) {
- int inc = (fb->format->depth == 15) ? 8 : 4;
- u8 r, b;
- for (i = 0; i < MGAG200_LUT_SIZE; i += inc) {
- if (fb->format->depth == 16) {
- if (i > (MGAG200_LUT_SIZE >> 1)) {
- r = b = 0;
- } else {
- r = *r_ptr++ >> 8;
- b = *b_ptr++ >> 8;
- r_ptr++;
- b_ptr++;
- }
- } else {
- r = *r_ptr++ >> 8;
- b = *b_ptr++ >> 8;
- }
- /* VGA registers */
- WREG8(DAC_INDEX + MGA1064_COL_PAL, r);
- WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8);
- WREG8(DAC_INDEX + MGA1064_COL_PAL, b);
+ switch (format) {
+ case DRM_FORMAT_RGB565:
+ /* Use better interpolation, to take 32 values from lut[0] to lut[255] */
+ for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i / 4].red >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i / 16].green >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i / 4].blue >> 8);
}
- return;
- }
- for (i = 0; i < MGAG200_LUT_SIZE; i++) {
- /* VGA registers */
- WREG8(DAC_INDEX + MGA1064_COL_PAL, *r_ptr++ >> 8);
- WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8);
- WREG8(DAC_INDEX + MGA1064_COL_PAL, *b_ptr++ >> 8);
+ /* Green has one more bit, so add padding with 0 for red and blue. */
+ for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i / 16].green >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
+ }
+ break;
+ case DRM_FORMAT_RGB888:
+ case DRM_FORMAT_XRGB8888:
+ for (i = 0; i < MGAG200_LUT_SIZE; i++) {
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
+ WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8);
+ }
+ break;
+ default:
+ drm_warn_once(&mdev->base, "Unsupported format for gamma %d\n", format);
+ break;
}
}
@@ -900,7 +919,11 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
if (mdev->type == G200_WB || mdev->type == G200_EW3)
mgag200_g200wb_release_bmc(mdev);
- mga_crtc_load_lut(crtc);
+ if (crtc_state->gamma_lut)
+ mgag200_crtc_set_gamma(mdev, crtc_state->gamma_lut->data, fb->format->format);
+ else
+ mgag200_crtc_set_gamma_linear(mdev, fb->format->format);
+
mgag200_enable_display(mdev);
mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);
@@ -945,6 +968,14 @@ mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
return ret;
}
+ if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
+ if (crtc_state->gamma_lut->length !=
+ MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) {
+ drm_err(dev, "Wrong size for gamma_lut %ld\n",
+ crtc_state->gamma_lut->length);
+ return -EINVAL;
+ }
+ }
return 0;
}
@@ -953,6 +984,7 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
struct drm_plane_state *old_state)
{
struct drm_plane *plane = &pipe->plane;
+ struct drm_crtc *crtc = &pipe->crtc;
struct drm_device *dev = plane->dev;
struct mga_device *mdev = to_mga_device(dev);
struct drm_plane_state *state = plane->state;
@@ -963,6 +995,9 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
if (!fb)
return;
+ if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
+ mgag200_crtc_set_gamma(mdev, crtc->state->gamma_lut->data, fb->format->format);
+
if (drm_atomic_helper_damage_merged(old_state, state, &damage))
mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]);
}
@@ -1107,9 +1142,11 @@ int mgag200_modeset_init(struct mga_device *mdev)
return ret;
}
- /* FIXME: legacy gamma tables; convert to CRTC state */
+ /* FIXME: legacy gamma tables, but atomic gamma doesn't work without */
drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);
+ drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE);
+
drm_mode_config_reset(dev);
return 0;
--
2.35.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] mgag200: Enable atomic gamma lut update
2022-05-11 15:28 [PATCH v2] mgag200: Enable atomic gamma lut update Jocelyn Falempe
@ 2022-05-11 21:14 ` kernel test robot
2022-05-11 22:26 ` kernel test robot
2022-05-12 8:52 ` Thomas Zimmermann
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-05-11 21:14 UTC (permalink / raw)
To: Jocelyn Falempe, dri-devel, lyude, tzimmermann
Cc: michel, kbuild-all, Jocelyn Falempe
Hi Jocelyn,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tegra-drm/drm/tegra/for-next]
[also build test WARNING on v5.18-rc6]
[cannot apply to drm/drm-next drm-tip/drm-tip airlied/drm-next next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/mgag200-Enable-atomic-gamma-lut-update/20220511-233134
base: git://anongit.freedesktop.org/tegra/linux.git drm/tegra/for-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220512/202205120525.DrSeu95X-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0831f1db9ae8814796efea603749709e80d2808c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jocelyn-Falempe/mgag200-Enable-atomic-gamma-lut-update/20220511-233134
git checkout 0831f1db9ae8814796efea603749709e80d2808c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/gpu/drm/mgag200/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/acpi.h:15,
from include/linux/i2c.h:13,
from include/drm/drm_crtc.h:28,
from include/drm/drm_atomic_helper.h:31,
from drivers/gpu/drm/mgag200/mgag200_mode.c:14:
drivers/gpu/drm/mgag200/mgag200_mode.c: In function 'mgag200_simple_display_pipe_check':
>> include/drm/drm_print.h:425:39: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
425 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
| ^~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
include/drm/drm_print.h:425:9: note: in expansion of macro 'dev_err'
425 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
| ^~~~
include/drm/drm_print.h:438:9: note: in expansion of macro '__drm_printk'
438 | __drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~
drivers/gpu/drm/mgag200/mgag200_mode.c:971:25: note: in expansion of macro 'drm_err'
971 | drm_err(dev, "Wrong size for gamma_lut %ld\n",
| ^~~~~~~
vim +425 include/drm/drm_print.h
02c9656b2f0d69 Haneen Mohammed 2017-10-17 385
02c9656b2f0d69 Haneen Mohammed 2017-10-17 386 /**
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 387 * DRM_DEV_DEBUG() - Debug output for generic drm code
02c9656b2f0d69 Haneen Mohammed 2017-10-17 388 *
306589856399e1 Douglas Anderson 2021-09-21 389 * NOTE: this is deprecated in favor of drm_dbg_core().
306589856399e1 Douglas Anderson 2021-09-21 390 *
091756bbb1a961 Haneen Mohammed 2017-10-17 391 * @dev: device pointer
091756bbb1a961 Haneen Mohammed 2017-10-17 392 * @fmt: printf() like format string.
02c9656b2f0d69 Haneen Mohammed 2017-10-17 393 */
db87086492581c Joe Perches 2018-03-16 394 #define DRM_DEV_DEBUG(dev, fmt, ...) \
db87086492581c Joe Perches 2018-03-16 395 drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 396 /**
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 397 * DRM_DEV_DEBUG_DRIVER() - Debug output for vendor specific part of the driver
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 398 *
306589856399e1 Douglas Anderson 2021-09-21 399 * NOTE: this is deprecated in favor of drm_dbg() or dev_dbg().
306589856399e1 Douglas Anderson 2021-09-21 400 *
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 401 * @dev: device pointer
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 402 * @fmt: printf() like format string.
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 403 */
db87086492581c Joe Perches 2018-03-16 404 #define DRM_DEV_DEBUG_DRIVER(dev, fmt, ...) \
db87086492581c Joe Perches 2018-03-16 405 drm_dev_dbg(dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 406 /**
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 407 * DRM_DEV_DEBUG_KMS() - Debug output for modesetting code
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 408 *
306589856399e1 Douglas Anderson 2021-09-21 409 * NOTE: this is deprecated in favor of drm_dbg_kms().
306589856399e1 Douglas Anderson 2021-09-21 410 *
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 411 * @dev: device pointer
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 412 * @fmt: printf() like format string.
b52817e9de06a3 Mauro Carvalho Chehab 2020-10-27 413 */
db87086492581c Joe Perches 2018-03-16 414 #define DRM_DEV_DEBUG_KMS(dev, fmt, ...) \
db87086492581c Joe Perches 2018-03-16 415 drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
a18b21929453af Lyude Paul 2018-07-16 416
fb6c7ab8718eb2 Jani Nikula 2019-12-10 417 /*
fb6c7ab8718eb2 Jani Nikula 2019-12-10 418 * struct drm_device based logging
fb6c7ab8718eb2 Jani Nikula 2019-12-10 419 *
fb6c7ab8718eb2 Jani Nikula 2019-12-10 420 * Prefer drm_device based logging over device or prink based logging.
fb6c7ab8718eb2 Jani Nikula 2019-12-10 421 */
fb6c7ab8718eb2 Jani Nikula 2019-12-10 422
fb6c7ab8718eb2 Jani Nikula 2019-12-10 423 /* Helper for struct drm_device based logging. */
fb6c7ab8718eb2 Jani Nikula 2019-12-10 424 #define __drm_printk(drm, level, type, fmt, ...) \
fb6c7ab8718eb2 Jani Nikula 2019-12-10 @425 dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
fb6c7ab8718eb2 Jani Nikula 2019-12-10 426
fb6c7ab8718eb2 Jani Nikula 2019-12-10 427
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] mgag200: Enable atomic gamma lut update
2022-05-11 15:28 [PATCH v2] mgag200: Enable atomic gamma lut update Jocelyn Falempe
@ 2022-05-11 22:26 ` kernel test robot
2022-05-11 22:26 ` kernel test robot
2022-05-12 8:52 ` Thomas Zimmermann
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-05-11 22:26 UTC (permalink / raw)
To: Jocelyn Falempe, dri-devel, lyude, tzimmermann
Cc: michel, llvm, kbuild-all, Jocelyn Falempe
Hi Jocelyn,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tegra-drm/drm/tegra/for-next]
[also build test WARNING on v5.18-rc6]
[cannot apply to drm/drm-next drm-tip/drm-tip airlied/drm-next next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/mgag200-Enable-atomic-gamma-lut-update/20220511-233134
base: git://anongit.freedesktop.org/tegra/linux.git drm/tegra/for-next
config: i386-randconfig-a003-20220509 (https://download.01.org/0day-ci/archive/20220512/202205120649.U2yM0PXz-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 18dd123c56754edf62c7042dcf23185c3727610f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0831f1db9ae8814796efea603749709e80d2808c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jocelyn-Falempe/mgag200-Enable-atomic-gamma-lut-update/20220511-233134
git checkout 0831f1db9ae8814796efea603749709e80d2808c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/mgag200/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/mgag200/mgag200_mode.c:972:5: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
crtc_state->gamma_lut->length);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/drm/drm_print.h:438:46: note: expanded from macro 'drm_err'
__drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/drm/drm_print.h:425:48: note: expanded from macro '__drm_printk'
dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:144:65: note: expanded from macro 'dev_err'
dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
_p_func(dev, fmt, ##__VA_ARGS__); \
~~~ ^~~~~~~~~~~
1 warning generated.
vim +972 drivers/gpu/drm/mgag200/mgag200_mode.c
937
938 static int
939 mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
940 struct drm_plane_state *plane_state,
941 struct drm_crtc_state *crtc_state)
942 {
943 struct drm_plane *plane = plane_state->plane;
944 struct drm_device *dev = plane->dev;
945 struct mga_device *mdev = to_mga_device(dev);
946 struct mgag200_pll *pixpll = &mdev->pixpll;
947 struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
948 struct drm_framebuffer *new_fb = plane_state->fb;
949 struct drm_framebuffer *fb = NULL;
950 int ret;
951
952 if (!new_fb)
953 return 0;
954
955 if (plane->state)
956 fb = plane->state->fb;
957
958 if (!fb || (fb->format != new_fb->format))
959 crtc_state->mode_changed = true; /* update PLL settings */
960
961 if (crtc_state->mode_changed) {
962 ret = pixpll->funcs->compute(pixpll, crtc_state->mode.clock,
963 &mgag200_crtc_state->pixpllc);
964 if (ret)
965 return ret;
966 }
967
968 if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
969 if (crtc_state->gamma_lut->length !=
970 MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) {
971 drm_err(dev, "Wrong size for gamma_lut %ld\n",
> 972 crtc_state->gamma_lut->length);
973 return -EINVAL;
974 }
975 }
976 return 0;
977 }
978
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] mgag200: Enable atomic gamma lut update
@ 2022-05-11 22:26 ` kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-05-11 22:26 UTC (permalink / raw)
To: Jocelyn Falempe, dri-devel, lyude, tzimmermann
Cc: llvm, kbuild-all, michel, Jocelyn Falempe
Hi Jocelyn,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tegra-drm/drm/tegra/for-next]
[also build test WARNING on v5.18-rc6]
[cannot apply to drm/drm-next drm-tip/drm-tip airlied/drm-next next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/mgag200-Enable-atomic-gamma-lut-update/20220511-233134
base: git://anongit.freedesktop.org/tegra/linux.git drm/tegra/for-next
config: i386-randconfig-a003-20220509 (https://download.01.org/0day-ci/archive/20220512/202205120649.U2yM0PXz-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 18dd123c56754edf62c7042dcf23185c3727610f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0831f1db9ae8814796efea603749709e80d2808c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jocelyn-Falempe/mgag200-Enable-atomic-gamma-lut-update/20220511-233134
git checkout 0831f1db9ae8814796efea603749709e80d2808c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/mgag200/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/mgag200/mgag200_mode.c:972:5: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
crtc_state->gamma_lut->length);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/drm/drm_print.h:438:46: note: expanded from macro 'drm_err'
__drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/drm/drm_print.h:425:48: note: expanded from macro '__drm_printk'
dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:144:65: note: expanded from macro 'dev_err'
dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
_p_func(dev, fmt, ##__VA_ARGS__); \
~~~ ^~~~~~~~~~~
1 warning generated.
vim +972 drivers/gpu/drm/mgag200/mgag200_mode.c
937
938 static int
939 mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
940 struct drm_plane_state *plane_state,
941 struct drm_crtc_state *crtc_state)
942 {
943 struct drm_plane *plane = plane_state->plane;
944 struct drm_device *dev = plane->dev;
945 struct mga_device *mdev = to_mga_device(dev);
946 struct mgag200_pll *pixpll = &mdev->pixpll;
947 struct mgag200_crtc_state *mgag200_crtc_state = to_mgag200_crtc_state(crtc_state);
948 struct drm_framebuffer *new_fb = plane_state->fb;
949 struct drm_framebuffer *fb = NULL;
950 int ret;
951
952 if (!new_fb)
953 return 0;
954
955 if (plane->state)
956 fb = plane->state->fb;
957
958 if (!fb || (fb->format != new_fb->format))
959 crtc_state->mode_changed = true; /* update PLL settings */
960
961 if (crtc_state->mode_changed) {
962 ret = pixpll->funcs->compute(pixpll, crtc_state->mode.clock,
963 &mgag200_crtc_state->pixpllc);
964 if (ret)
965 return ret;
966 }
967
968 if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
969 if (crtc_state->gamma_lut->length !=
970 MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) {
971 drm_err(dev, "Wrong size for gamma_lut %ld\n",
> 972 crtc_state->gamma_lut->length);
973 return -EINVAL;
974 }
975 }
976 return 0;
977 }
978
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mgag200: Enable atomic gamma lut update
2022-05-11 15:28 [PATCH v2] mgag200: Enable atomic gamma lut update Jocelyn Falempe
2022-05-11 21:14 ` kernel test robot
2022-05-11 22:26 ` kernel test robot
@ 2022-05-12 8:52 ` Thomas Zimmermann
2022-05-12 10:08 ` Jocelyn Falempe
2 siblings, 1 reply; 6+ messages in thread
From: Thomas Zimmermann @ 2022-05-12 8:52 UTC (permalink / raw)
To: Jocelyn Falempe, dri-devel, lyude; +Cc: michel
[-- Attachment #1.1: Type: text/plain, Size: 8615 bytes --]
Hi
Am 11.05.22 um 17:28 schrieb Jocelyn Falempe:
> Add support for atomic update of gamma lut.
> With this patch the "Night light" feature of gnome3
> is working properly on mgag200.
>
> v2:
> - Add a default linear gamma function
> - renamed functions with mgag200 prefix
> - use format's 4cc code instead of bit depth
> - use better interpolation for 16bits gamma
> - remove legacy function mga_crtc_load_lut()
> - can't remove the call to drm_mode_crtc_set_gamma_size()
> because it doesn't work with userspace.
> - other small refactors
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
I already gave a Tested-by on the first iteration. It's good practice to
add these tags in follow-up patches unless the patch has changed entirely.
A few more comments are below. With those fixed:
Reviewed-by: Thomas Zimmermann <tzimemrmann@suse.de>
I suggest to post another version of the patch and merge it if no
further comments arrive within 2 days.
> ---
> drivers/gpu/drm/mgag200/mgag200_mode.c | 125 ++++++++++++++++---------
> 1 file changed, 81 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 6e18d3bbd720..b748bc5b0e93 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -32,57 +32,76 @@
> * This file contains setup code for the CRTC.
> */
>
> -static void mga_crtc_load_lut(struct drm_crtc *crtc)
> +static void mgag200_crtc_set_gamma_linear(struct mga_device *mdev,
> + uint32_t format)
> {
> - struct drm_device *dev = crtc->dev;
> - struct mga_device *mdev = to_mga_device(dev);
> - struct drm_framebuffer *fb;
> - u16 *r_ptr, *g_ptr, *b_ptr;
> int i;
>
> - if (!crtc->enabled)
> - return;
> -
> - if (!mdev->display_pipe.plane.state)
> - return;
> + WREG8(DAC_INDEX + MGA1064_INDEX, 0);
>
> - fb = mdev->display_pipe.plane.state->fb;
> + switch (format) {
> + case DRM_FORMAT_RGB565:
> + /* Use better interpolation, to take 32 values from 0 to 255 */
> + for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) {
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4);
> + }
> + /* Green has one more bit, so add padding with 0 for red and blue. */
> + for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) {
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
> + }
> + break;
> + case DRM_FORMAT_RGB888:
> + case DRM_FORMAT_XRGB8888:
> + for (i = 0; i < MGAG200_LUT_SIZE; i++) {
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
> + }
These loops look much nicer to me.
> + break;
> + default:
> + drm_warn_once(&mdev->base, "Unsupported format for gamma %d\n", format);
There's a print format modifier for 4cc formats. It's %p4cc and expects
a pointer to the format's 4cc code. See 'git grep p4cc' for examples.
The comment itself is not easy to understand. Maybe "Unsupported format
%p4cc for gamma correction.\n" ?
> + break;
> + }
> +}
>
> - r_ptr = crtc->gamma_store;
> - g_ptr = r_ptr + crtc->gamma_size;
> - b_ptr = g_ptr + crtc->gamma_size;
> +static void mgag200_crtc_set_gamma(struct mga_device *mdev,
> + struct drm_color_lut *lut,
> + uint32_t format)
> +{
> + int i;
>
> WREG8(DAC_INDEX + MGA1064_INDEX, 0);
>
> - if (fb && fb->format->cpp[0] * 8 == 16) {
> - int inc = (fb->format->depth == 15) ? 8 : 4;
> - u8 r, b;
> - for (i = 0; i < MGAG200_LUT_SIZE; i += inc) {
> - if (fb->format->depth == 16) {
> - if (i > (MGAG200_LUT_SIZE >> 1)) {
> - r = b = 0;
> - } else {
> - r = *r_ptr++ >> 8;
> - b = *b_ptr++ >> 8;
> - r_ptr++;
> - b_ptr++;
> - }
> - } else {
> - r = *r_ptr++ >> 8;
> - b = *b_ptr++ >> 8;
> - }
> - /* VGA registers */
> - WREG8(DAC_INDEX + MGA1064_COL_PAL, r);
> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8);
> - WREG8(DAC_INDEX + MGA1064_COL_PAL, b);
> + switch (format) {
> + case DRM_FORMAT_RGB565:
> + /* Use better interpolation, to take 32 values from lut[0] to lut[255] */
> + for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) {
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i / 4].red >> 8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i / 16].green >> 8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i / 4].blue >> 8);
> }
> - return;
> - }
> - for (i = 0; i < MGAG200_LUT_SIZE; i++) {
> - /* VGA registers */
> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *r_ptr++ >> 8);
> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8);
> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *b_ptr++ >> 8);
> + /* Green has one more bit, so add padding with 0 for red and blue. */
> + for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) {
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i / 16].green >> 8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
> + }
> + break;
> + case DRM_FORMAT_RGB888:
> + case DRM_FORMAT_XRGB8888:
> + for (i = 0; i < MGAG200_LUT_SIZE; i++) {
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8);
> + }
> + break;
> + default:
> + drm_warn_once(&mdev->base, "Unsupported format for gamma %d\n", format);
Same as above.
> + break;
> }
> }
>
> @@ -900,7 +919,11 @@ mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
> if (mdev->type == G200_WB || mdev->type == G200_EW3)
> mgag200_g200wb_release_bmc(mdev);
>
> - mga_crtc_load_lut(crtc);
> + if (crtc_state->gamma_lut)
> + mgag200_crtc_set_gamma(mdev, crtc_state->gamma_lut->data, fb->format->format);
Nitpicking: I'd give the format before the LUT data. It's more logical
and aligns with '_set_gamma_linear'. I'd also pass fb->format instead of
fb->format->format.
> + else
> + mgag200_crtc_set_gamma_linear(mdev, fb->format->format);
> +
> mgag200_enable_display(mdev);
>
> mgag200_handle_damage(mdev, fb, &fullscreen, &shadow_plane_state->data[0]);
> @@ -945,6 +968,14 @@ mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,
> return ret;
> }
>
> + if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
> + if (crtc_state->gamma_lut->length !=
> + MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) {
> + drm_err(dev, "Wrong size for gamma_lut %ld\n",
The kernel bot complained about '%ld'. I think %zu is the one for size_t.
> + crtc_state->gamma_lut->length);
> + return -EINVAL;
> + }
> + }
> return 0;
> }
>
> @@ -953,6 +984,7 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
> struct drm_plane_state *old_state)
> {
> struct drm_plane *plane = &pipe->plane;
> + struct drm_crtc *crtc = &pipe->crtc;
> struct drm_device *dev = plane->dev;
> struct mga_device *mdev = to_mga_device(dev);
> struct drm_plane_state *state = plane->state;
> @@ -963,6 +995,9 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
> if (!fb)
> return;
>
> + if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
> + mgag200_crtc_set_gamma(mdev, crtc->state->gamma_lut->data, fb->format->format);
> +
This also needs a call to _set_gamma_linear?
Best regards
Thomas
> if (drm_atomic_helper_damage_merged(old_state, state, &damage))
> mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]);
> }
> @@ -1107,9 +1142,11 @@ int mgag200_modeset_init(struct mga_device *mdev)
> return ret;
> }
>
> - /* FIXME: legacy gamma tables; convert to CRTC state */
> + /* FIXME: legacy gamma tables, but atomic gamma doesn't work without */
> drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);
>
> + drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE);
> +
> drm_mode_config_reset(dev);
>
> return 0;
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] mgag200: Enable atomic gamma lut update
2022-05-12 8:52 ` Thomas Zimmermann
@ 2022-05-12 10:08 ` Jocelyn Falempe
0 siblings, 0 replies; 6+ messages in thread
From: Jocelyn Falempe @ 2022-05-12 10:08 UTC (permalink / raw)
To: Thomas Zimmermann, dri-devel, lyude; +Cc: michel
On 12/05/2022 10:52, Thomas Zimmermann wrote:
> Hi
>
> Am 11.05.22 um 17:28 schrieb Jocelyn Falempe:
>> Add support for atomic update of gamma lut.
>> With this patch the "Night light" feature of gnome3
>> is working properly on mgag200.
>>
>> v2:
>> - Add a default linear gamma function
>> - renamed functions with mgag200 prefix
>> - use format's 4cc code instead of bit depth
>> - use better interpolation for 16bits gamma
>> - remove legacy function mga_crtc_load_lut()
>> - can't remove the call to drm_mode_crtc_set_gamma_size()
>> because it doesn't work with userspace.
>> - other small refactors
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>
> I already gave a Tested-by on the first iteration. It's good practice to
> add these tags in follow-up patches unless the patch has changed entirely.
Sorry, I though I changed too much code in v2 to add it.
>
> A few more comments are below. With those fixed:
>
> Reviewed-by: Thomas Zimmermann <tzimemrmann@suse.de>
>
> I suggest to post another version of the patch and merge it if no
> further comments arrive within 2 days.
>
>> ---
>> drivers/gpu/drm/mgag200/mgag200_mode.c | 125 ++++++++++++++++---------
>> 1 file changed, 81 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index 6e18d3bbd720..b748bc5b0e93 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -32,57 +32,76 @@
>> * This file contains setup code for the CRTC.
>> */
>> -static void mga_crtc_load_lut(struct drm_crtc *crtc)
>> +static void mgag200_crtc_set_gamma_linear(struct mga_device *mdev,
>> + uint32_t format)
>> {
>> - struct drm_device *dev = crtc->dev;
>> - struct mga_device *mdev = to_mga_device(dev);
>> - struct drm_framebuffer *fb;
>> - u16 *r_ptr, *g_ptr, *b_ptr;
>> int i;
>> - if (!crtc->enabled)
>> - return;
>> -
>> - if (!mdev->display_pipe.plane.state)
>> - return;
>> + WREG8(DAC_INDEX + MGA1064_INDEX, 0);
>> - fb = mdev->display_pipe.plane.state->fb;
>> + switch (format) {
>> + case DRM_FORMAT_RGB565:
>> + /* Use better interpolation, to take 32 values from 0 to 255 */
>> + for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) {
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 8 + i / 4);
>> + }
>> + /* Green has one more bit, so add padding with 0 for red and
>> blue. */
>> + for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) {
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i * 4 + i / 16);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
>> + }
>> + break;
>> + case DRM_FORMAT_RGB888:
>> + case DRM_FORMAT_XRGB8888:
>> + for (i = 0; i < MGAG200_LUT_SIZE; i++) {
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, i);
>> + }
>
> These loops look much nicer to me.
>
>> + break;
>> + default:
>> + drm_warn_once(&mdev->base, "Unsupported format for gamma
>> %d\n", format);
>
> There's a print format modifier for 4cc formats. It's %p4cc and expects
> a pointer to the format's 4cc code. See 'git grep p4cc' for examples.
ok, that's a cool feature.
>
> The comment itself is not easy to understand. Maybe "Unsupported format
> %p4cc for gamma correction.\n" ?
Sure, having good error message is always helpful.
>
>> + break;
>> + }
>> +}
>> - r_ptr = crtc->gamma_store;
>> - g_ptr = r_ptr + crtc->gamma_size;
>> - b_ptr = g_ptr + crtc->gamma_size;
>> +static void mgag200_crtc_set_gamma(struct mga_device *mdev,
>> + struct drm_color_lut *lut,
>> + uint32_t format)
>> +{
>> + int i;
>> WREG8(DAC_INDEX + MGA1064_INDEX, 0);
>> - if (fb && fb->format->cpp[0] * 8 == 16) {
>> - int inc = (fb->format->depth == 15) ? 8 : 4;
>> - u8 r, b;
>> - for (i = 0; i < MGAG200_LUT_SIZE; i += inc) {
>> - if (fb->format->depth == 16) {
>> - if (i > (MGAG200_LUT_SIZE >> 1)) {
>> - r = b = 0;
>> - } else {
>> - r = *r_ptr++ >> 8;
>> - b = *b_ptr++ >> 8;
>> - r_ptr++;
>> - b_ptr++;
>> - }
>> - } else {
>> - r = *r_ptr++ >> 8;
>> - b = *b_ptr++ >> 8;
>> - }
>> - /* VGA registers */
>> - WREG8(DAC_INDEX + MGA1064_COL_PAL, r);
>> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8);
>> - WREG8(DAC_INDEX + MGA1064_COL_PAL, b);
>> + switch (format) {
>> + case DRM_FORMAT_RGB565:
>> + /* Use better interpolation, to take 32 values from lut[0] to
>> lut[255] */
>> + for (i = 0; i < MGAG200_LUT_SIZE / 8; i++) {
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i / 4].red
>> >> 8);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i /
>> 16].green >> 8);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 8 + i /
>> 4].blue >> 8);
>> }
>> - return;
>> - }
>> - for (i = 0; i < MGAG200_LUT_SIZE; i++) {
>> - /* VGA registers */
>> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *r_ptr++ >> 8);
>> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *g_ptr++ >> 8);
>> - WREG8(DAC_INDEX + MGA1064_COL_PAL, *b_ptr++ >> 8);
>> + /* Green has one more bit, so add padding with 0 for red and
>> blue. */
>> + for (i = MGAG200_LUT_SIZE / 8; i < MGAG200_LUT_SIZE / 4; i++) {
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i * 4 + i /
>> 16].green >> 8);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, 0);
>> + }
>> + break;
>> + case DRM_FORMAT_RGB888:
>> + case DRM_FORMAT_XRGB8888:
>> + for (i = 0; i < MGAG200_LUT_SIZE; i++) {
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8);
>> + }
>> + break;
>> + default:
>> + drm_warn_once(&mdev->base, "Unsupported format for gamma
>> %d\n", format);
>
> Same as above.
>
>> + break;
>> }
>> }
>> @@ -900,7 +919,11 @@ mgag200_simple_display_pipe_enable(struct
>> drm_simple_display_pipe *pipe,
>> if (mdev->type == G200_WB || mdev->type == G200_EW3)
>> mgag200_g200wb_release_bmc(mdev);
>> - mga_crtc_load_lut(crtc);
>> + if (crtc_state->gamma_lut)
>> + mgag200_crtc_set_gamma(mdev, crtc_state->gamma_lut->data,
>> fb->format->format);
>
> Nitpicking: I'd give the format before the LUT data. It's more logical
> and aligns with '_set_gamma_linear'. I'd also pass fb->format instead of
> fb->format->format.
ok, I will do that too.
>
>> + else
>> + mgag200_crtc_set_gamma_linear(mdev, fb->format->format);
>> +
>> mgag200_enable_display(mdev);
>> mgag200_handle_damage(mdev, fb, &fullscreen,
>> &shadow_plane_state->data[0]);
>> @@ -945,6 +968,14 @@ mgag200_simple_display_pipe_check(struct
>> drm_simple_display_pipe *pipe,
>> return ret;
>> }
>> + if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
>> + if (crtc_state->gamma_lut->length !=
>> + MGAG200_LUT_SIZE * sizeof(struct drm_color_lut)) {
>> + drm_err(dev, "Wrong size for gamma_lut %ld\n",
>
> The kernel bot complained about '%ld'. I think %zu is the one for size_t.
I had no warnings when building on x86_64, but printf format is a bit
tricky to get right.
>
>> + crtc_state->gamma_lut->length);
>> + return -EINVAL;
>> + }
>> + }
>> return 0;
>> }
>> @@ -953,6 +984,7 @@ mgag200_simple_display_pipe_update(struct
>> drm_simple_display_pipe *pipe,
>> struct drm_plane_state *old_state)
>> {
>> struct drm_plane *plane = &pipe->plane;
>> + struct drm_crtc *crtc = &pipe->crtc;
>> struct drm_device *dev = plane->dev;
>> struct mga_device *mdev = to_mga_device(dev);
>> struct drm_plane_state *state = plane->state;
>> @@ -963,6 +995,9 @@ mgag200_simple_display_pipe_update(struct
>> drm_simple_display_pipe *pipe,
>> if (!fb)
>> return;
>> + if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
>> + mgag200_crtc_set_gamma(mdev, crtc->state->gamma_lut->data,
>> fb->format->format);
>> +
>
> This also needs a call to _set_gamma_linear?
No, I think the gamma table should be properly initialized in
mgag200_simple_display_pipe_enable(), so only set it if userspace gives
a new table.
>
> Best regards
> Thomas
>
>> if (drm_atomic_helper_damage_merged(old_state, state, &damage))
>> mgag200_handle_damage(mdev, fb, &damage,
>> &shadow_plane_state->data[0]);
>> }
>> @@ -1107,9 +1142,11 @@ int mgag200_modeset_init(struct mga_device *mdev)
>> return ret;
>> }
>> - /* FIXME: legacy gamma tables; convert to CRTC state */
>> + /* FIXME: legacy gamma tables, but atomic gamma doesn't work
>> without */
>> drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);
>> + drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE);
>> +
>> drm_mode_config_reset(dev);
>> return 0;
>
Thanks for your review, I will send a v3 soon,
--
Jocelyn
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-12 10:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-11 15:28 [PATCH v2] mgag200: Enable atomic gamma lut update Jocelyn Falempe
2022-05-11 21:14 ` kernel test robot
2022-05-11 22:26 ` kernel test robot
2022-05-11 22:26 ` kernel test robot
2022-05-12 8:52 ` Thomas Zimmermann
2022-05-12 10:08 ` Jocelyn Falempe
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.