On 10/12/2020 17:27, Daniel Vetter wrote:
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index e82db0f4e771..80e3797f0f01 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -46,6 +46,7 @@ #include <drm/drm_print.h> #include <drm/drm_vblank.h>
+#include "drm_crtc_internal.h"
So this is a bit annoying, because thus far we managed to have a very clear split between core and helpers. And I think we can keep that.
#include "drm_crtc_helper_internal.h" #include "drm_internal.h"
@@ -136,15 +137,15 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) { uint16_t *r_base, *g_base, *b_base;
- if (crtc->funcs->gamma_set == NULL)
if (!drm_crtc_supports_legacy_gamma(crtc)) return;
r_base = crtc->gamma_store; g_base = r_base + crtc->gamma_size; b_base = g_base + crtc->gamma_size;
- crtc->funcs->gamma_set(crtc, r_base, g_base, b_base,
crtc->gamma_size, NULL);
- drm_crtc_legacy_gamma_set(crtc, r_base, g_base, b_base,
crtc->gamma_size, NULL);This is only used by legacy non-atomic drivers. It's pretty much impossible to make kgdb work with atomic drivers, so really let's just not bother and keep the code as-is.
You're right.
}
/** @@ -946,7 +947,7 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) drm_modeset_lock_all(fb_helper->dev); drm_client_for_each_modeset(modeset, &fb_helper->client) { crtc = modeset->crtc;
if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
if (!drm_crtc_supports_legacy_gamma(crtc)) { ret = -EINVAL; goto out;@@ -964,8 +965,8 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
ret = crtc->funcs->gamma_set(crtc, r, g, b,crtc->gamma_size, NULL);
ret = drm_crtc_legacy_gamma_set(crtc, r, g, b, crtc->gamma_size,NULL);Same here.
Yep.
} @@ -1024,12 +1025,10 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) struct drm_device *dev = fb_helper->dev; struct drm_property_blob *gamma_lut = NULL; struct drm_modeset_acquire_ctx ctx;
struct drm_crtc_state *crtc_state; struct drm_atomic_state *state; struct drm_mode_set *modeset; struct drm_crtc *crtc; u16 *r, *g, *b;
bool replaced; int ret = 0;
drm_modeset_acquire_init(&ctx, 0);
@@ -1053,18 +1052,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) goto out_state; }
crtc_state = drm_atomic_get_crtc_state(state, crtc);if (IS_ERR(crtc_state)) {ret = PTR_ERR(crtc_state);
ret = drm_crtc_gamma_ramp_set(state, crtc, gamma_lut);if (ret)You're nesting an atomic commit in an atomic commit here, that will go boom. I guess ideally we'd move this into drm_client_modeset so it remembers the fbdev gamma ramp and does it all in one go. Otherwise I guess you need some kind of different helper, not sure what.
What do you mean? Are you mixing drm_crtc_legacy_gamma_set with drm_crtc_gamma_ramp_set (yeah, I didn't quite know how to name the latter one...)? drm_crtc_gamma_ramp_set does the same thing as the removed code, it sets the gamma_lut in to the state. It doesn't commit.
drm_crtc_gamma_ramp_set does a "setup the state so that this gamma ramp will be on screen", which means setting/clearing GAMMA_LUT, DEGAMMA_LUT and CTM. I wanted to have that logic in one place, which means we need to export it from drm.ko.
I could just inline drm_crtc_gamma_ramp_set, but then I need drm_mode_obj_find_prop_id, which is again not exported. I could also inline drm_mode_obj_find_prop_id as it's trivial enough loop. But this sounds uglier than exporting a function.
Personally, I don't remember when I have used fbdev the last time (other than simple tests), and I could as well just leave the code here as it is. I have no idea if this fbdev setcmap is a big feature that has to function also with HW that only has a pre-gamma table.
Tomi