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