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
