| Message ID | 20220511152815.892562-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mgag200: Enable atomic gamma lut update | expand |
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
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
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;
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,
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;
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(-)