| Message ID | 20220509094930.44613-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mgag200: Enable atomic gamma lut update | expand |
Hi, first of all Tested-by: Thomas Zimemrmann <tzimmermann@suse.de> on G200EH. I clicked a bit in Gnome settings and the display changed colors. It's pretty cool. Am 09.05.22 um 11:49 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. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/mgag200/mgag200_mode.c | 46 ++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 6e18d3bbd720..9fc688e15db8 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -86,6 +86,46 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) mga_crtc_load_lut is legacy code and needs to go away. > } > } > > +static void mga_crtc_update_lut(struct mga_device *mdev, > + struct drm_crtc_state *state, > + u8 depth) Rather name this function mgag200_crtc_set_gamma(). The driver was once ported from X11 userspace, where it was called mga. Thus the occational mga_ prefix. It it should now be mgag200. > +{ > + struct drm_color_lut * lut; > + int i; > + > + if (!state->color_mgmt_changed || !state->gamma_lut) > + return Semicolon is missing here. The test itself should go into the caller. The update function here should be independent from the crtc state. Pass in the plane state's framebuffer and the crtc state's gamma_lut property. > + > + lut = (struct drm_color_lut *) state->gamma_lut->data; > + WREG8(DAC_INDEX + MGA1064_INDEX, 0); > + > + if (depth == 15) { format->depth is deprecated. Better write these if-else branches as switch of the format's 4cc code: switch (fb->format->format) { case DRM_FORMAT_XRGB1555: ... break; case DRM_FORMAT_RGB565: ... break; case DRM_FORMAT_RGB888: case DRM_FORMAT_XRGB: ... break; } > + /* 16 bits r5g5b5a1 */ With 4cc codes, you can remove these comments. > + for (i = 0; i < MGAG200_LUT_SIZE; i += 8) { > + 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); > + } > + } else if (depth == 16) { > + /* 16 bits r5g6b5, as green has one more bit, > + * add padding with 0 for red and blue. */ > + for (i = 0; i < MGAG200_LUT_SIZE; i += 4) { > + u8 red = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0; > + u8 blue = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0; '[].blue' here. > + WREG8(DAC_INDEX + MGA1064_COL_PAL, red); > + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8); > + WREG8(DAC_INDEX + MGA1064_COL_PAL, blue); > + } > + } else { > + /* 24 bits r8g8b8 */ > + 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); > + } > + } > +} > + These loops seem hard to understand because the index i doesn't obviously correspond to the source or destination; except for the final one. I'd write out the offset into the HW palette as constant value in the for loop and walk over the given lut table via pointer arithmetic. It might also make sense to adjust the starting value of the lut table such that its final entry is used for the final entry in the HW palette. For typical gamma ramps ~2, the curve is fairly flat for small values and goes up steeply at high values. (Please correct me if I'm misinterpreting the gamma ramps.) For 15-bit case I'd do thing like this. lut += 7; for (i < 0; i < 32; ++i, lut += 8) { // write lut } 16-bit is complicated and may better be done in 2 loops lutr += 7; lutg += 3; lutb += 7; for (i < 0; i < 32; ++i, lutr += 8, lutg += 3, lutb += 8) { // write r/g/b lut } for (; i < 64; ++i, lutg += 3) { // write 0/g/0 lut } > static inline void mga_wait_vsync(struct mga_device *mdev) > { > unsigned long timeout = jiffies + HZ/10; > @@ -953,6 +993,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,7 +1004,10 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, > if (!fb) > return; > > + mga_crtc_update_lut(mdev, crtc->state, fb->format->depth); > + We should also call this function in pipe_enable. And there's the question what happens if gamma_lut is not set. So far, we get away with it because mga_crtc_load_lut(). A better approach is to add another function mgag200_crtc_set_gamma_linear() that clears the palette to a linear curve (i.e., same as mga_crtc_load_lut() does now). It would be called if no crtc->state->gamma_lut is NULL. > if (drm_atomic_helper_damage_merged(old_state, state, &damage)) > + That empty line is fallout from rebasing from the other patchset? > mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]); > } > > @@ -1110,6 +1154,8 @@ int mgag200_modeset_init(struct mga_device *mdev) > /* FIXME: legacy gamma tables; convert to CRTC state */ > drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE); Here's another legacy call that should get removed. > > + drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE); AFAICT the DRM core does not enforce the LUT size. It's just information for userspace, which could give any amount of palette entries. So the driver's pipe_check function has to enforce the limit. If the gamma_lut property is set, it should always contain 256 entries. Best regards Thomas > + > drm_mode_config_reset(dev); > > return 0;
On 09/05/2022 16:22, Thomas Zimmermann wrote: > Hi, > > first of all > > Tested-by: Thomas Zimemrmann <tzimmermann@suse.de> > > on G200EH. I clicked a bit in Gnome settings and the display changed > colors. It's pretty cool. yeah, I also played a bit with https://github.com/zb3/gnome-gamma-tool > > Am 09.05.22 um 11:49 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. >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/mgag200/mgag200_mode.c | 46 ++++++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c >> b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index 6e18d3bbd720..9fc688e15db8 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >> @@ -86,6 +86,46 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) > > mga_crtc_load_lut is legacy code and needs to go away. > >> } >> } >> +static void mga_crtc_update_lut(struct mga_device *mdev, >> + struct drm_crtc_state *state, >> + u8 depth) > > Rather name this function mgag200_crtc_set_gamma(). > > The driver was once ported from X11 userspace, where it was called mga. > Thus the occational mga_ prefix. It it should now be mgag200. ok > >> +{ >> + struct drm_color_lut * lut; >> + int i; >> + >> + if (!state->color_mgmt_changed || !state->gamma_lut) >> + return > > Semicolon is missing here. oops ;) > > The test itself should go into the caller. The update function here > should be independent from the crtc state. Pass in the plane state's > framebuffer and the crtc state's gamma_lut property. ok, it makes sense. > >> + >> + lut = (struct drm_color_lut *) state->gamma_lut->data; >> + WREG8(DAC_INDEX + MGA1064_INDEX, 0); >> + >> + if (depth == 15) { > > format->depth is deprecated. Better write these if-else branches as > switch of the format's 4cc code: > > switch (fb->format->format) { > case DRM_FORMAT_XRGB1555: > ... > break; > case DRM_FORMAT_RGB565: > ... > break; > case DRM_FORMAT_RGB888: > case DRM_FORMAT_XRGB: > ... > break; > } As the driver doesn't advertise XRGB1555, maybe I can drop it ? I kept it because the mga_crtc_load_lut() supports it. > >> + /* 16 bits r5g5b5a1 */ > > With 4cc codes, you can remove these comments. > >> + for (i = 0; i < MGAG200_LUT_SIZE; i += 8) { >> + 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); >> + } >> + } else if (depth == 16) { >> + /* 16 bits r5g6b5, as green has one more bit, >> + * add padding with 0 for red and blue. */ >> + for (i = 0; i < MGAG200_LUT_SIZE; i += 4) { >> + u8 red = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0; >> + u8 blue = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 >> : 0; > > '[].blue' here. oops again ;) > >> + WREG8(DAC_INDEX + MGA1064_COL_PAL, red); >> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8); >> + WREG8(DAC_INDEX + MGA1064_COL_PAL, blue); >> + } >> + } else { >> + /* 24 bits r8g8b8 */ >> + 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); >> + } >> + } >> +} >> + > > These loops seem hard to understand because the index i doesn't > obviously correspond to the source or destination; except for the final > one. > > I'd write out the offset into the HW palette as constant value in the > for loop and walk over the given lut table via pointer arithmetic. > > It might also make sense to adjust the starting value of the lut table > such that its final entry is used for the final entry in the HW palette. > For typical gamma ramps ~2, the curve is fairly flat for small values > and goes up steeply at high values. (Please correct me if I'm > misinterpreting the gamma ramps.) I didn't realize that taking 1 out of 8 values will have this side effect. sure this can be fixed. > > For 15-bit case I'd do thing like this. > > lut += 7; > for (i < 0; i < 32; ++i, lut += 8) { > // write lut > } > > 16-bit is complicated and may better be done in 2 loops > > lutr += 7; > lutg += 3; > lutb += 7; > for (i < 0; i < 32; ++i, lutr += 8, lutg += 3, lutb += 8) { > // write r/g/b lut > } > for (; i < 64; ++i, lutg += 3) { > // write 0/g/0 lut > } ok, will try to do something like this. It took me a while to understand the loops of mga_crtc_load_lut(), so I tried to simplify. > >> static inline void mga_wait_vsync(struct mga_device *mdev) >> { >> unsigned long timeout = jiffies + HZ/10; >> @@ -953,6 +993,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,7 +1004,10 @@ mgag200_simple_display_pipe_update(struct >> drm_simple_display_pipe *pipe, >> if (!fb) >> return; >> + mga_crtc_update_lut(mdev, crtc->state, fb->format->depth); >> + > > We should also call this function in pipe_enable. > > And there's the question what happens if gamma_lut is not set. So far, > we get away with it because mga_crtc_load_lut(). A better approach is > to add another function mgag200_crtc_set_gamma_linear() that clears the > palette to a linear curve (i.e., same as mga_crtc_load_lut() does now). > It would be called if no crtc->state->gamma_lut is NULL. Yes, if I remove mga_crtc_load_lut() I will need to set the default. should be simple to do. > >> if (drm_atomic_helper_damage_merged(old_state, state, &damage)) >> + > > That empty line is fallout from rebasing from the other patchset? yes ;) > >> mgag200_handle_damage(mdev, fb, &damage, >> &shadow_plane_state->data[0]); >> } >> @@ -1110,6 +1154,8 @@ int mgag200_modeset_init(struct mga_device *mdev) >> /* FIXME: legacy gamma tables; convert to CRTC state */ >> drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE); > > Here's another legacy call that should get removed. Yes, I can remove that one too. > >> + drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, MGAG200_LUT_SIZE); > > AFAICT the DRM core does not enforce the LUT size. It's just information > for userspace, which could give any amount of palette entries. So the > driver's pipe_check function has to enforce the limit. If the gamma_lut > property is set, it should always contain 256 entries. > > Best regards > Thomas > >> + >> drm_mode_config_reset(dev); >> return 0; > Thanks a lot for your review, I will rework this and send a v2.
On 2022-05-09 16:22, Thomas Zimmermann wrote: > > It might also make sense to adjust the starting value of the lut table such that its final entry is used for the final entry in the HW palette. For typical gamma ramps ~2, the curve is fairly flat for small values and goes up steeply at high values. (Please correct me if I'm misinterpreting the gamma ramps.) I don't think that's accurate. The most common ramp should be a straight line from 0 to the maximum value, and others may be curved toward the top or bottom. > For 15-bit case I'd do thing like this. > > lut += 7; > for (i < 0; i < 32; ++i, lut += 8) { > // write lut > } > > 16-bit is complicated and may better be done in 2 loops > > lutr += 7; > lutg += 3; > lutb += 7; > for (i < 0; i < 32; ++i, lutr += 8, lutg += 3, lutb += 8) { > // write r/g/b lut > } > for (; i < 64; ++i, lutg += 3) { > // write 0/g/0 lut > } That'll just drop the first 3-7 entries of the LUT instead of the last ones, i.e. generally the full black entries instead of the full white ones. Ideally, the loop should start at 0 and then count as evenly as possible up to 255/63/31. I realize that's tricky though, and I don't have any specific suggestions for how to achieve this offhand.
On 09/05/2022 18:04, Michel Dänzer wrote: > On 2022-05-09 16:22, Thomas Zimmermann wrote: >> >> It might also make sense to adjust the starting value of the lut table such that its final entry is used for the final entry in the HW palette. For typical gamma ramps ~2, the curve is fairly flat for small values and goes up steeply at high values. (Please correct me if I'm misinterpreting the gamma ramps.) > > I don't think that's accurate. The most common ramp should be a straight line from 0 to the maximum value, and others may be curved toward the top or bottom. > > >> For 15-bit case I'd do thing like this. >> >> lut += 7; >> for (i < 0; i < 32; ++i, lut += 8) { >> // write lut >> } >> >> 16-bit is complicated and may better be done in 2 loops >> >> lutr += 7; >> lutg += 3; >> lutb += 7; >> for (i < 0; i < 32; ++i, lutr += 8, lutg += 3, lutb += 8) { >> // write r/g/b lut >> } >> for (; i < 64; ++i, lutg += 3) { >> // write 0/g/0 lut >> } > > That'll just drop the first 3-7 entries of the LUT instead of the last ones, i.e. generally the full black entries instead of the full white ones. > > Ideally, the loop should start at 0 and then count as evenly as possible up to 255/63/31. I realize that's tricky though, and I don't have any specific suggestions for how to achieve this offhand. > > If you want 32 values from the 256 table, something like this should work: for (i=0; i<32; i++) { lut_index = i * 8 + i / 4; } lut_index will have this value: 0, 8, 16, 24, 33, 41, 49, 57, 66, 74, 82, 90, 99, 107, 115, 123, 132, 140, 148, 156, 165, 173, 181, 189, 198, 206, 214, 222, 231, 239, 247, 255
Hi Jocelyn,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tegra-drm/drm/tegra/for-next]
[also build test ERROR on v5.18-rc6]
[cannot apply to drm/drm-next drm-tip/drm-tip airlied/drm-next next-20220509]
[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/20220509-175430
base: git://anongit.freedesktop.org/tegra/linux.git drm/tegra/for-next
config: mips-randconfig-r015-20220509 (https://download.01.org/0day-ci/archive/20220510/202205100516.IJQ7MRHW-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a385645b470e2d3a1534aae618ea56b31177639f)
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
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/c0e0a39a5acd3aa9d0cd6f25679bd16930233491
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/20220509-175430
git checkout c0e0a39a5acd3aa9d0cd6f25679bd16930233491
# 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=mips 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 errors (new ones prefixed by >>):
>> drivers/gpu/drm/mgag200/mgag200_mode.c:97:3: error: void function 'mga_crtc_update_lut' should not return a value [-Wreturn-type]
return
^
1 error generated.
vim +/mga_crtc_update_lut +97 drivers/gpu/drm/mgag200/mgag200_mode.c
88
89 static void mga_crtc_update_lut(struct mga_device *mdev,
90 struct drm_crtc_state *state,
91 u8 depth)
92 {
93 struct drm_color_lut * lut;
94 int i;
95
96 if (!state->color_mgmt_changed || !state->gamma_lut)
> 97 return
98
99 lut = (struct drm_color_lut *) state->gamma_lut->data;
100 WREG8(DAC_INDEX + MGA1064_INDEX, 0);
101
102 if (depth == 15) {
103 /* 16 bits r5g5b5a1 */
104 for (i = 0; i < MGAG200_LUT_SIZE; i += 8) {
105 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8);
106 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
107 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8);
108 }
109 } else if (depth == 16) {
110 /* 16 bits r5g6b5, as green has one more bit,
111 * add padding with 0 for red and blue. */
112 for (i = 0; i < MGAG200_LUT_SIZE; i += 4) {
113 u8 red = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0;
114 u8 blue = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0;
115 WREG8(DAC_INDEX + MGA1064_COL_PAL, red);
116 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
117 WREG8(DAC_INDEX + MGA1064_COL_PAL, blue);
118 }
119 } else {
120 /* 24 bits r8g8b8 */
121 for (i = 0; i < MGAG200_LUT_SIZE; i++) {
122 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].red >> 8);
123 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8);
124 WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].blue >> 8);
125 }
126 }
127 }
128
Hi Am 09.05.22 um 23:00 schrieb Jocelyn Falempe: > On 09/05/2022 18:04, Michel Dänzer wrote: >> On 2022-05-09 16:22, Thomas Zimmermann wrote: >>> >>> It might also make sense to adjust the starting value of the lut >>> table such that its final entry is used for the final entry in the HW >>> palette. For typical gamma ramps ~2, the curve is fairly flat for >>> small values and goes up steeply at high values. (Please correct me >>> if I'm misinterpreting the gamma ramps.) >> >> I don't think that's accurate. The most common ramp should be a >> straight line from 0 to the maximum value, and others may be curved >> toward the top or bottom. >> >> >>> For 15-bit case I'd do thing like this. >>> >>> lut += 7; >>> for (i < 0; i < 32; ++i, lut += 8) { >>> // write lut >>> } >>> >>> 16-bit is complicated and may better be done in 2 loops >>> >>> lutr += 7; >>> lutg += 3; >>> lutb += 7; >>> for (i < 0; i < 32; ++i, lutr += 8, lutg += 3, lutb += 8) { >>> // write r/g/b lut >>> } >>> for (; i < 64; ++i, lutg += 3) { >>> // write 0/g/0 lut >>> } >> >> That'll just drop the first 3-7 entries of the LUT instead of the last >> ones, i.e. generally the full black entries instead of the full white >> ones. >> >> Ideally, the loop should start at 0 and then count as evenly as >> possible up to 255/63/31. I realize that's tricky though, and I don't >> have any specific suggestions for how to achieve this offhand. >> >> > > If you want 32 values from the 256 table, something like this should work: > > for (i=0; i<32; i++) { > lut_index = i * 8 + i / 4; > } > > lut_index will have this value: > > 0, 8, 16, 24, 33, 41, 49, 57, 66, 74, 82, 90, 99, 107, 115, 123, 132, > 140, 148, 156, 165, 173, 181, 189, 198, 206, 214, 222, 231, 239, 247, 255 Great. That's even better. Best regards Thomas > >
Hi Am 09.05.22 um 17:43 schrieb Jocelyn Falempe: > On 09/05/2022 16:22, Thomas Zimmermann wrote: >> Hi, >> >> first of all >> >> Tested-by: Thomas Zimemrmann <tzimmermann@suse.de> >> >> on G200EH. I clicked a bit in Gnome settings and the display changed >> colors. It's pretty cool. > > yeah, I also played a bit with https://github.com/zb3/gnome-gamma-tool > >> >> Am 09.05.22 um 11:49 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. >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> --- >>> drivers/gpu/drm/mgag200/mgag200_mode.c | 46 ++++++++++++++++++++++++++ >>> 1 file changed, 46 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c >>> b/drivers/gpu/drm/mgag200/mgag200_mode.c >>> index 6e18d3bbd720..9fc688e15db8 100644 >>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >>> @@ -86,6 +86,46 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) >> >> mga_crtc_load_lut is legacy code and needs to go away. >> >>> } >>> } >>> +static void mga_crtc_update_lut(struct mga_device *mdev, >>> + struct drm_crtc_state *state, >>> + u8 depth) >> >> Rather name this function mgag200_crtc_set_gamma(). >> >> The driver was once ported from X11 userspace, where it was called >> mga. Thus the occational mga_ prefix. It it should now be mgag200. > > ok >> >>> +{ >>> + struct drm_color_lut * lut; >>> + int i; >>> + >>> + if (!state->color_mgmt_changed || !state->gamma_lut) >>> + return >> >> Semicolon is missing here. > > oops ;) >> >> The test itself should go into the caller. The update function here >> should be independent from the crtc state. Pass in the plane state's >> framebuffer and the crtc state's gamma_lut property. > > ok, it makes sense. >> >>> + >>> + lut = (struct drm_color_lut *) state->gamma_lut->data; >>> + WREG8(DAC_INDEX + MGA1064_INDEX, 0); >>> + >>> + if (depth == 15) { >> >> format->depth is deprecated. Better write these if-else branches as >> switch of the format's 4cc code: >> >> switch (fb->format->format) { >> case DRM_FORMAT_XRGB1555: >> ... >> break; >> case DRM_FORMAT_RGB565: >> ... >> break; >> case DRM_FORMAT_RGB888: >> case DRM_FORMAT_XRGB: >> ... >> break; >> } > > As the driver doesn't advertise XRGB1555, maybe I can drop it ? > I kept it because the mga_crtc_load_lut() supports it. If you want to remove it, go ahead. If we'd want to support 15-bit modes, it would be trivial to add. > >> >>> + /* 16 bits r5g5b5a1 */ >> >> With 4cc codes, you can remove these comments. >> >>> + for (i = 0; i < MGAG200_LUT_SIZE; i += 8) { >>> + 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); >>> + } >>> + } else if (depth == 16) { >>> + /* 16 bits r5g6b5, as green has one more bit, >>> + * add padding with 0 for red and blue. */ >>> + for (i = 0; i < MGAG200_LUT_SIZE; i += 4) { >>> + u8 red = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 >>> : 0; >>> + u8 blue = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 >>> : 0; >> >> '[].blue' here. > > oops again ;) > >> >>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, red); >>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8); >>> + WREG8(DAC_INDEX + MGA1064_COL_PAL, blue); >>> + } >>> + } else { >>> + /* 24 bits r8g8b8 */ >>> + 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); >>> + } >>> + } >>> +} >>> + >> >> These loops seem hard to understand because the index i doesn't >> obviously correspond to the source or destination; except for the >> final one. >> >> I'd write out the offset into the HW palette as constant value in the >> for loop and walk over the given lut table via pointer arithmetic. >> >> It might also make sense to adjust the starting value of the lut table >> such that its final entry is used for the final entry in the HW >> palette. For typical gamma ramps ~2, the curve is fairly flat for >> small values and goes up steeply at high values. (Please correct me if >> I'm misinterpreting the gamma ramps.) > > I didn't realize that taking 1 out of 8 values will have this side > effect. sure this can be fixed. I'm looking at gamma curves as given in [1]. The typical display gamma of ~2 gives a exponential-looking graph, which starts flat but grows quickly. Michel is right that we ideally what full-dark and full-bright as-is, and do some interpolation in between. In your reply to Michel, I think you already gave a simple and correct-enough implementation. I'd just go with that. Best regards Thomas [1] https://www.cambridgeincolour.com/tutorials/gamma-correction.htm >> >> For 15-bit case I'd do thing like this. >> >> lut += 7; >> for (i < 0; i < 32; ++i, lut += 8) { >> // write lut >> } >> >> 16-bit is complicated and may better be done in 2 loops >> >> lutr += 7; >> lutg += 3; >> lutb += 7; >> for (i < 0; i < 32; ++i, lutr += 8, lutg += 3, lutb += 8) { >> // write r/g/b lut >> } >> for (; i < 64; ++i, lutg += 3) { >> // write 0/g/0 lut >> } > > ok, will try to do something like this. It took me a while to understand > the loops of mga_crtc_load_lut(), so I tried to simplify. >> >>> static inline void mga_wait_vsync(struct mga_device *mdev) >>> { >>> unsigned long timeout = jiffies + HZ/10; >>> @@ -953,6 +993,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,7 +1004,10 @@ mgag200_simple_display_pipe_update(struct >>> drm_simple_display_pipe *pipe, >>> if (!fb) >>> return; >>> + mga_crtc_update_lut(mdev, crtc->state, fb->format->depth); >>> + >> >> We should also call this function in pipe_enable. >> >> And there's the question what happens if gamma_lut is not set. So >> far, we get away with it because mga_crtc_load_lut(). A better >> approach is to add another function mgag200_crtc_set_gamma_linear() >> that clears the palette to a linear curve (i.e., same as >> mga_crtc_load_lut() does now). It would be called if no >> crtc->state->gamma_lut is NULL. > > Yes, if I remove mga_crtc_load_lut() I will need to set the default. > should be simple to do. > > >> >>> if (drm_atomic_helper_damage_merged(old_state, state, &damage)) >>> + >> >> That empty line is fallout from rebasing from the other patchset? > > yes ;) >> >>> mgag200_handle_damage(mdev, fb, &damage, >>> &shadow_plane_state->data[0]); >>> } >>> @@ -1110,6 +1154,8 @@ int mgag200_modeset_init(struct mga_device *mdev) >>> /* FIXME: legacy gamma tables; convert to CRTC state */ >>> drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE); >> >> Here's another legacy call that should get removed. > > Yes, I can remove that one too. > >> >>> + drm_crtc_enable_color_mgmt(&pipe->crtc, 0, false, >>> MGAG200_LUT_SIZE); >> >> AFAICT the DRM core does not enforce the LUT size. It's just >> information for userspace, which could give any amount of palette >> entries. So the driver's pipe_check function has to enforce the limit. >> If the gamma_lut property is set, it should always contain 256 entries. >> >> Best regards >> Thomas >> >>> + >>> drm_mode_config_reset(dev); >>> return 0; >> > > Thanks a lot for your review, I will rework this and send a v2. >
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 6e18d3bbd720..9fc688e15db8 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -86,6 +86,46 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) } } +static void mga_crtc_update_lut(struct mga_device *mdev, + struct drm_crtc_state *state, + u8 depth) +{ + struct drm_color_lut * lut; + int i; + + if (!state->color_mgmt_changed || !state->gamma_lut) + return + + lut = (struct drm_color_lut *) state->gamma_lut->data; + WREG8(DAC_INDEX + MGA1064_INDEX, 0); + + if (depth == 15) { + /* 16 bits r5g5b5a1 */ + for (i = 0; i < MGAG200_LUT_SIZE; i += 8) { + 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); + } + } else if (depth == 16) { + /* 16 bits r5g6b5, as green has one more bit, + * add padding with 0 for red and blue. */ + for (i = 0; i < MGAG200_LUT_SIZE; i += 4) { + u8 red = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0; + u8 blue = 2 * i < MGAG200_LUT_SIZE ? lut[2 * i].red >> 8 : 0; + WREG8(DAC_INDEX + MGA1064_COL_PAL, red); + WREG8(DAC_INDEX + MGA1064_COL_PAL, lut[i].green >> 8); + WREG8(DAC_INDEX + MGA1064_COL_PAL, blue); + } + } else { + /* 24 bits r8g8b8 */ + 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); + } + } +} + static inline void mga_wait_vsync(struct mga_device *mdev) { unsigned long timeout = jiffies + HZ/10; @@ -953,6 +993,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,7 +1004,10 @@ mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, if (!fb) return; + mga_crtc_update_lut(mdev, crtc->state, fb->format->depth); + if (drm_atomic_helper_damage_merged(old_state, state, &damage)) + mgag200_handle_damage(mdev, fb, &damage, &shadow_plane_state->data[0]); } @@ -1110,6 +1154,8 @@ int mgag200_modeset_init(struct mga_device *mdev) /* FIXME: legacy gamma tables; convert to CRTC state */ 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. Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/mgag200/mgag200_mode.c | 46 ++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)