[Intel-gfx] [PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Jun 11 18:08:39 UTC 2019
On Tue, Jun 11, 2019 at 07:55:45PM +0200, Daniel Vetter wrote:
> On Tue, Jun 11, 2019 at 7:50 PM Ville Syrjälä
> <ville.syrjala at linux.intel.com> 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 at linux.intel.com>
> > > >
> > > > 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 at linux.intel.com>
> > > > ---
> > > > 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.
Yeah, I think everyone would just pass the size from getcrtc and so
we'd end up rejecting anything else.
Ah yes, that is pretty much the only way to use the getgamma ioctl
since it doesn't even support the 'size=0 -> just query the actual
size without returning any other data' approach used in other places.
--
Ville Syrjälä
Intel
More information about the dri-devel
mailing list