On Thu, Dec 10, 2020 at 4:43 PM Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
>
> 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.
I mean my brain's not working today at all :-/ You're right.
> 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.
I have honestly no idea how much this is used. Maybe just throw a
FIXME comment on top of this here that it doesn't handle degamma and
should be moved into drm_client_modeset.c? And then leave the code
unchanged, we're not breaking anything by not implementing degamma
support for fbdev. Whomever cares can address that in a neat and clean
fashion.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
