Re: [PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 11, 2019 at 7:50 PM Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Jun 07, 2019 at 08:40:15PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 07, 2019 at 07:26:10PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > i915 will now refuse to enable a C8 plane unless the gamma_lut
> > > is already enabled (to avoid scanning out whatever garbage got
> > > left in the hw LUT previously). So in order to make the fbdev
> > > code work with C8 we need to program the gamma_lut already
> > > during restore_fbdev_mode_atomic().
> > >
> > > To avoid having to update the hw LUT every time
> > > restore_fbdev_mode_atomic() is called we hang on to the
> > > current gamma_lut blob. Note that the first crtc to get
> > > enabled will dictate the size of the gamma_lut, so this
> > > approach isn't 100% great for hardware with non-uniform
> > > crtcs. But that problem already exists in setcmap_atomic()
> > > so we'll just keep ignoring it.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/drm_fb_helper.c | 165 ++++++++++++++++++++------------
> > >  include/drm/drm_fb_helper.h     |   7 ++
> > >  2 files changed, 113 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index bdfa14cd7f6d..0ecec763789f 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -436,10 +436,76 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
> > >     return true;
> > >  }
> > >
> > > +static int setcmap_crtc_atomic(struct drm_atomic_state *state,
> > > +                          struct drm_crtc *crtc,
> > > +                          struct drm_property_blob *gamma_lut)
> > > +{
> > > +   struct drm_crtc_state *crtc_state;
> > > +   bool replaced;
> > > +
> > > +   crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > > +   if (IS_ERR(crtc_state))
> > > +           return PTR_ERR(crtc_state);
> > > +
> > > +   replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > > +                                         NULL);
> > > +   replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > > +   replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > > +                                         gamma_lut);
> > > +   crtc_state->color_mgmt_changed |= replaced;
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > > +                                                  struct fb_cmap *cmap)
> > > +{
> > > +   struct drm_device *dev = crtc->dev;
> > > +   struct drm_property_blob *gamma_lut;
> > > +   struct drm_color_lut *lut;
> > > +   int size = crtc->gamma_size;
> > > +   int i;
> > > +
> > > +   if (!size || cmap->start + cmap->len > size)
> > > +           return ERR_PTR(-EINVAL);
> > > +
> > > +   gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > > +   if (IS_ERR(gamma_lut))
> > > +           return gamma_lut;
> > > +
> > > +   lut = gamma_lut->data;
> > > +   if (cmap->start || cmap->len != size) {
> > > +           u16 *r = crtc->gamma_store;
> > > +           u16 *g = r + crtc->gamma_size;
> > > +           u16 *b = g + crtc->gamma_size;
> > > +
> > > +           for (i = 0; i < cmap->start; i++) {
> > > +                   lut[i].red = r[i];
> > > +                   lut[i].green = g[i];
> > > +                   lut[i].blue = b[i];
> > > +           }
> > > +           for (i = cmap->start + cmap->len; i < size; i++) {
> > > +                   lut[i].red = r[i];
> > > +                   lut[i].green = g[i];
> > > +                   lut[i].blue = b[i];
> > > +           }
> > > +   }
> > > +
> > > +   for (i = 0; i < cmap->len; i++) {
> > > +           lut[cmap->start + i].red = cmap->red[i];
> > > +           lut[cmap->start + i].green = cmap->green[i];
> > > +           lut[cmap->start + i].blue = cmap->blue[i];
> > > +   }
> > > +
> > > +   return gamma_lut;
> > > +}
> > > +
> > >  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
> > >  {
> > > +   struct fb_info *info = fb_helper->fbdev;
> > >     struct drm_client_dev *client = &fb_helper->client;
> > >     struct drm_device *dev = fb_helper->dev;
> > > +   struct drm_property_blob *gamma_lut;
> > >     struct drm_plane_state *plane_state;
> > >     struct drm_plane *plane;
> > >     struct drm_atomic_state *state;
> > > @@ -455,6 +521,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >             goto out_ctx;
> > >     }
> > >
> > > +   gamma_lut = fb_helper->gamma_lut;
> > > +   if (gamma_lut)
> > > +           drm_property_blob_get(gamma_lut);
> >
> > Why the get/put stuff here?
>
> So we don't free this guy during the cleanup.
>
> >
> > > +
> > >     state->acquire_ctx = &ctx;
> > >  retry:
> > >     drm_for_each_plane(plane, dev) {
> > > @@ -476,7 +546,8 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >     }
> > >
> > >     drm_client_for_each_modeset(mode_set, client) {
> > > -           struct drm_plane *primary = mode_set->crtc->primary;
> > > +           struct drm_crtc *crtc = mode_set->crtc;
> > > +           struct drm_plane *primary = crtc->primary;
> > >             unsigned int rotation;
> > >
> > >             if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
> > > @@ -489,24 +560,46 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >             if (ret != 0)
> > >                     goto out_state;
> > >
> > > +           if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> > > +                   if (!gamma_lut)
> > > +                           gamma_lut = setcmap_new_gamma_lut(crtc, &info->cmap);
> > > +                   if (IS_ERR(gamma_lut)) {
> > > +                           ret = PTR_ERR(gamma_lut);
> > > +                           gamma_lut = NULL;
> > > +                           goto out_state;
> > > +                   }
> > > +
> > > +                   ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > > +                   if (ret)
> > > +                           goto out_state;
> > > +           }
> > > +
> > >             /*
> > >              * __drm_atomic_helper_set_config() sets active when a
> > >              * mode is set, unconditionally clear it if we force DPMS off
> > >              */
> > >             if (!active) {
> > > -                   struct drm_crtc *crtc = mode_set->crtc;
> > > -                   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > +                   struct drm_crtc_state *crtc_state =
> > > +                           drm_atomic_get_new_crtc_state(state, crtc);
> > >
> > >                     crtc_state->active = false;
> > >             }
> > >     }
> > >
> > >     ret = drm_atomic_commit(state);
> > > +   if (ret)
> > > +           goto out_state;
> > > +
> > > +   if (gamma_lut)
> > > +           drm_property_blob_get(gamma_lut);
> > > +   drm_property_blob_put(fb_helper->gamma_lut);
> > > +   fb_helper->gamma_lut = gamma_lut;
> >
> > Can't we simplify using drm_property_replace_blob here and below?
>
> I guess.
>
> >
> > >
> > >  out_state:
> > >     if (ret == -EDEADLK)
> > >             goto backoff;
> > >
> > > +   drm_property_blob_put(gamma_lut);
> > >     drm_atomic_state_put(state);
> > >  out_ctx:
> > >     drm_modeset_drop_locks(&ctx);
> > > @@ -996,6 +1089,9 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> > >     }
> > >     fb_helper->fbdev = NULL;
> > >
> > > +   drm_property_blob_put(fb_helper->gamma_lut);
> > > +   fb_helper->gamma_lut = NULL;
> > > +
> > >     mutex_lock(&kernel_fb_helper_lock);
> > >     if (!list_empty(&fb_helper->kernel_fb_list)) {
> > >             list_del(&fb_helper->kernel_fb_list);
> > > @@ -1390,61 +1486,16 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
> > >     return ret;
> > >  }
> > >
> > > -static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> > > -                                                  struct fb_cmap *cmap)
> > > -{
> > > -   struct drm_device *dev = crtc->dev;
> > > -   struct drm_property_blob *gamma_lut;
> > > -   struct drm_color_lut *lut;
> > > -   int size = crtc->gamma_size;
> > > -   int i;
> > > -
> > > -   if (!size || cmap->start + cmap->len > size)
> > > -           return ERR_PTR(-EINVAL);
> > > -
> > > -   gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> > > -   if (IS_ERR(gamma_lut))
> > > -           return gamma_lut;
> > > -
> > > -   lut = gamma_lut->data;
> > > -   if (cmap->start || cmap->len != size) {
> > > -           u16 *r = crtc->gamma_store;
> > > -           u16 *g = r + crtc->gamma_size;
> > > -           u16 *b = g + crtc->gamma_size;
> > > -
> > > -           for (i = 0; i < cmap->start; i++) {
> > > -                   lut[i].red = r[i];
> > > -                   lut[i].green = g[i];
> > > -                   lut[i].blue = b[i];
> > > -           }
> > > -           for (i = cmap->start + cmap->len; i < size; i++) {
> > > -                   lut[i].red = r[i];
> > > -                   lut[i].green = g[i];
> > > -                   lut[i].blue = b[i];
> > > -           }
> > > -   }
> > > -
> > > -   for (i = 0; i < cmap->len; i++) {
> > > -           lut[cmap->start + i].red = cmap->red[i];
> > > -           lut[cmap->start + i].green = cmap->green[i];
> > > -           lut[cmap->start + i].blue = cmap->blue[i];
> > > -   }
> > > -
> > > -   return gamma_lut;
> > > -}
> > > -
> > >  static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > >  {
> > >     struct drm_fb_helper *fb_helper = info->par;
> > >     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);
> > > @@ -1468,18 +1519,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 = setcmap_crtc_atomic(state, crtc, gamma_lut);
> > > +           if (ret)
> > >                     goto out_state;
> > > -           }
> > > -
> > > -           replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> > > -                                                 NULL);
> > > -           replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> > > -           replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> > > -                                                 gamma_lut);
> > > -           crtc_state->color_mgmt_changed |= replaced;
> > >     }
> > >
> > >     ret = drm_atomic_commit(state);
> > > @@ -1498,6 +1540,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
> > >             memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
> > >     }
> >
> > The above "update legacy color table" stuff is missing from the
> > restore_fbdev_mode_atomic version.
>
> If the cmap didn't change in the meantime there should be nothing to
> update, maybe. Though I suppose if someone did a gamma_set in the
> meantime we might be out of sync a bit again.
>
> I was actually thinking of nuking this legacy gamma_store stuff
> entirely for atomic drivers. It's already out of sync with the
> GAMMA_LUT prop anyway so seems mostly dead weight.
>
> > Unify more, i.e. maybe convert
> > setcmap_atomic to just update the property in fb_helper->gamma_lut, and
> > then call into restore_fbdev_mode_atomic to do all of the heavy lifting?
>
> Hmm. Yeah, that seems like it could work.
>
> >
> > Or maybe we should push this into
> > drm_atomic_helper_update_legacy_modeset_state(), but that doesn't quite
> > work because locking rules are silly :-/ Or teach the gamma_get ioctl to
> > look at atomic and ditch this, dunno.
>
> Yeah, that was my thinking for getting rid of gamma_store. It would
> also fix the out of sync issue between get_gamma and GAMMA_LUT.
> I suppose I should just do it instead of procrastinating.
>
> There are a few open questions though:
> - what if the current LUT is the wrong size?
> - what if there is no LUT?
>
> I see two options:
> 1) Lie and always return something with crtc->gamma_size
>    entries, ie. just duplicate/drop entries for the wrong size,
>    and just return a linear LUT when there is no LUT actually
>    loaded
> 2) Expose the truth and figure out if existing userspace
>    could actually handle it. Quick scan shows that most users
>    would just ignore the existing LUT in this case.

I guess in practice this would amount to

if (crtc_lut->gamma_size == crtc->gamma_size == crtc->state->gamma_blob.size)
return atomic gamma table convert to legacy formate
else
return EINVAL;

I think this could work. I think in practice the only case we need to
make sure keeps working is when userspace uses legacy gamma
exclusively. If you have a mixed world it's a mess already anyway.
-Daniel

>
> Option 1 seems safer.
>
> --
> Ville Syrjälä
> Intel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux