[PATCH v3 1/2] drm: automatic legacy gamma support

[Daniel Vetter]

On Thu, Dec 10, 2020 at 4:43 PM Tomi Valkeinen <tomi.valkeinen at ti.com> 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