From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754090AbdGNNy3 (ORCPT ); Fri, 14 Jul 2017 09:54:29 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33283 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753599AbdGNNy2 (ORCPT ); Fri, 14 Jul 2017 09:54:28 -0400 Date: Fri, 14 Jul 2017 15:54:24 +0200 From: Daniel Vetter To: Peter Rosin Cc: linux-kernel@vger.kernel.org, Boris Brezillon , dri-devel@lists.freedesktop.org, Daniel Vetter Subject: Re: [PATCH v5 03/14] drm/fb-helper: separate the fb_setcmap helper into atomic and legacy paths Message-ID: <20170714135423.f4uodzns24p7u675@phenom.ffwll.local> Mail-Followup-To: Peter Rosin , linux-kernel@vger.kernel.org, Boris Brezillon , dri-devel@lists.freedesktop.org, Daniel Vetter References: <20170713162538.22788-1-peda@axentia.se> <20170713162538.22788-4-peda@axentia.se> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170713162538.22788-4-peda@axentia.se> X-Operating-System: Linux phenom 4.11.0-1-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 13, 2017 at 06:25:27PM +0200, Peter Rosin wrote: > The legacy path implements setcmap in terms of crtc .gamma_set. > > The atomic path implements setcmap by directly updating the crtc gamma_lut > property. > > This has a couple of benefits: > - it makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get > completely obsolete. They are now unused and subject for removal. > - atomic drivers that support clut modes get fbdev support for those from > the drm core. This includes atmel-hlcdc, but perhaps others as well? > > Signed-off-by: Peter Rosin Ok, I merged the core parts. I'll wait with the driver stuff for a bit more (maybe 1-2 weeks) for more acks. Pls remind me in case I forget to pull them in. Thanks a lot for doing this, great work! -Daniel > --- > drivers/gpu/drm/drm_fb_helper.c | 231 ++++++++++++++++++++++++++++------------ > 1 file changed, 160 insertions(+), 71 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 721511da4de6..42090fe00ef9 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1195,27 +1195,6 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper, > } > EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked); > > -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green, > - u16 blue, u16 regno, struct fb_info *info) > -{ > - struct drm_fb_helper *fb_helper = info->par; > - struct drm_framebuffer *fb = fb_helper->fb; > - > - /* > - * The driver really shouldn't advertise pseudo/directcolor > - * visuals if it can't deal with the palette. > - */ > - if (WARN_ON(!fb_helper->funcs->gamma_set || > - !fb_helper->funcs->gamma_get)) > - return -EINVAL; > - > - WARN_ON(fb->format->cpp[0] != 1); > - > - fb_helper->funcs->gamma_set(crtc, red, green, blue, regno); > - > - return 0; > -} > - > static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) > { > u32 *palette = (u32 *)info->pseudo_palette; > @@ -1248,57 +1227,138 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) > return 0; > } > > -/** > - * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap > - * @cmap: cmap to set > - * @info: fbdev registered by the helper > - */ > -int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > +static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) > { > struct drm_fb_helper *fb_helper = info->par; > - struct drm_device *dev = fb_helper->dev; > - const struct drm_crtc_helper_funcs *crtc_funcs; > - u16 *red, *green, *blue, *transp; > struct drm_crtc *crtc; > u16 *r, *g, *b; > - int i, j, rc = 0; > - int start; > + int i, ret = 0; > > - if (oops_in_progress) > - return -EBUSY; > + drm_modeset_lock_all(fb_helper->dev); > + for (i = 0; i < fb_helper->crtc_count; i++) { > + crtc = fb_helper->crtc_info[i].mode_set.crtc; > + if (!crtc->funcs->gamma_set || !crtc->gamma_size) > + return -EINVAL; > > - mutex_lock(&fb_helper->lock); > - if (!drm_fb_helper_is_bound(fb_helper)) { > - mutex_unlock(&fb_helper->lock); > - return -EBUSY; > + if (cmap->start + cmap->len > crtc->gamma_size) > + return -EINVAL; > + > + r = crtc->gamma_store; > + g = r + crtc->gamma_size; > + b = g + crtc->gamma_size; > + > + memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(*r)); > + 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); > + if (ret) > + return ret; > } > + drm_modeset_unlock_all(fb_helper->dev); > > - drm_modeset_lock_all(dev); > - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { > - rc = setcmap_pseudo_palette(cmap, info); > - goto out; > + 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 = (struct drm_color_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_crtc *crtc; > + u16 *r, *g, *b; > + int i, ret = 0; > + bool replaced; > + > + drm_modeset_acquire_init(&ctx, 0); > + > + state = drm_atomic_state_alloc(dev); > + if (!state) { > + ret = -ENOMEM; > + goto out_ctx; > } > > + state->acquire_ctx = &ctx; > +retry: > for (i = 0; i < fb_helper->crtc_count; i++) { > crtc = fb_helper->crtc_info[i].mode_set.crtc; > - crtc_funcs = crtc->helper_private; > > - red = cmap->red; > - green = cmap->green; > - blue = cmap->blue; > - transp = cmap->transp; > - start = cmap->start; > - > - if (!crtc->gamma_size) { > - rc = -EINVAL; > - goto out; > + if (!gamma_lut) > + gamma_lut = setcmap_new_gamma_lut(crtc, cmap); > + if (IS_ERR(gamma_lut)) { > + ret = PTR_ERR(gamma_lut); > + gamma_lut = NULL; > + goto out_state; > } > > - if (cmap->start + cmap->len > crtc->gamma_size) { > - rc = -EINVAL; > - goto out; > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) { > + ret = PTR_ERR(crtc_state); > + 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); > + if (ret) > + goto out_state; > + > + for (i = 0; i < fb_helper->crtc_count; i++) { > + crtc = fb_helper->crtc_info[i].mode_set.crtc; > + > r = crtc->gamma_store; > g = r + crtc->gamma_size; > b = g + crtc->gamma_size; > @@ -1306,28 +1366,57 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(*r)); > memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); > memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b)); > + } > + > +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); > + drm_modeset_acquire_fini(&ctx); > > - for (j = 0; j < cmap->len; j++) { > - u16 hred, hgreen, hblue, htransp = 0xffff; > + return ret; > > - hred = *red++; > - hgreen = *green++; > - hblue = *blue++; > +backoff: > + drm_atomic_state_clear(state); > + drm_modeset_backoff(&ctx); > + goto retry; > +} > > - if (transp) > - htransp = *transp++; > +/** > + * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap > + * @cmap: cmap to set > + * @info: fbdev registered by the helper > + */ > +int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + int ret; > > - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info); > - if (rc) > - goto out; > - } > - if (crtc_funcs->load_lut) > - crtc_funcs->load_lut(crtc); > + if (oops_in_progress) > + return -EBUSY; > + > + mutex_lock(&fb_helper->lock); > + > + if (!drm_fb_helper_is_bound(fb_helper)) { > + ret = -EBUSY; > + goto out; > } > - out: > - drm_modeset_unlock_all(dev); > + > + if (info->fix.visual == FB_VISUAL_TRUECOLOR) > + ret = setcmap_pseudo_palette(cmap, info); > + else if (drm_drv_uses_atomic_modeset(fb_helper->dev)) > + ret = setcmap_atomic(cmap, info); > + else > + ret = setcmap_legacy(cmap, info); > + > +out: > mutex_unlock(&fb_helper->lock); > - return rc; > + > + return ret; > } > EXPORT_SYMBOL(drm_fb_helper_setcmap); > > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch