| Message ID | 1499343648-29695-4-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 06, 2017 at 02:20:37PM +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 <peda@axentia.se> > --- > drivers/gpu/drm/drm_fb_helper.c | 232 ++++++++++++++++++++++++++++------------ > 1 file changed, 161 insertions(+), 71 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 721511d..32d6ea1 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,140 @@ 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; > + 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; > + > + 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; > + bool replaced = false; > > - red = cmap->red; > - green = cmap->green; > - blue = cmap->blue; > - transp = cmap->transp; > - start = cmap->start; > + crtc = fb_helper->crtc_info[i].mode_set.crtc; > > - if (!crtc->gamma_size) { > - rc = -EINVAL; > - goto out; > + gamma_lut = setcmap_new_gamma_lut(crtc, cmap); Tiny nit you might want to improve (since you need to respin for my naming bikeshed of the property_replace_blob function anyway): Properties are refcounting and invariant, which means you can just create the property once, and then use it for all the CRTC. Slightly cleaner code. Otherwise I've carefully reviewed this, and it all looks perfect now. Thanks, Daniel > + if (IS_ERR(gamma_lut)) { > + ret = PTR_ERR(gamma_lut); > + 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)) { > + drm_property_blob_put(gamma_lut); > + ret = PTR_ERR(crtc_state); > + goto out_state; > } > > + drm_atomic_replace_property_blob(&crtc_state->degamma_lut, > + NULL, &replaced); > + drm_atomic_replace_property_blob(&crtc_state->ctm, > + NULL, &replaced); > + drm_atomic_replace_property_blob(&crtc_state->gamma_lut, > + gamma_lut, &replaced); > + crtc_state->color_mgmt_changed |= replaced; > + drm_property_blob_put(gamma_lut); > + } > + > + 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 +1368,56 @@ 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_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.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2017-07-11 10:10, Daniel Vetter wrote: > On Thu, Jul 06, 2017 at 02:20:37PM +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 <peda@axentia.se> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 232 ++++++++++++++++++++++++++++------------ >> 1 file changed, 161 insertions(+), 71 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 721511d..32d6ea1 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,140 @@ 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; >> + 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; >> + >> + 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; >> + bool replaced = false; >> >> - red = cmap->red; >> - green = cmap->green; >> - blue = cmap->blue; >> - transp = cmap->transp; >> - start = cmap->start; >> + crtc = fb_helper->crtc_info[i].mode_set.crtc; >> >> - if (!crtc->gamma_size) { >> - rc = -EINVAL; >> - goto out; >> + gamma_lut = setcmap_new_gamma_lut(crtc, cmap); > > Tiny nit you might want to improve (since you need to respin for my naming > bikeshed of the property_replace_blob function anyway): Properties are > refcounting and invariant, which means you can just create the property > once, and then use it for all the CRTC. Slightly cleaner code. Yes, I thought about that, but ended up not. The reason is that as far as I could tell, all involved crtc need not have the same original gamma_lut. Sure, if all crtc have the same history, that should be the case, but isn't it possible to tie things together one way first and set some clut, then "rewire" things so that the crtc no longer have the same history? But if you in the light of that still think it's wise to set the same clut for all crtc I will factor that part out of the loop. Cheers, peda > Otherwise I've carefully reviewed this, and it all looks perfect now. > > Thanks, Daniel > >> + if (IS_ERR(gamma_lut)) { >> + ret = PTR_ERR(gamma_lut); >> + 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)) { >> + drm_property_blob_put(gamma_lut); >> + ret = PTR_ERR(crtc_state); >> + goto out_state; >> } >> >> + drm_atomic_replace_property_blob(&crtc_state->degamma_lut, >> + NULL, &replaced); >> + drm_atomic_replace_property_blob(&crtc_state->ctm, >> + NULL, &replaced); >> + drm_atomic_replace_property_blob(&crtc_state->gamma_lut, >> + gamma_lut, &replaced); >> + crtc_state->color_mgmt_changed |= replaced; >> + drm_property_blob_put(gamma_lut); >> + } >> + >> + 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 +1368,56 @@ 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_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.1.4 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, Jul 11, 2017 at 02:12:26PM +0200, Peter Rosin wrote: > On 2017-07-11 10:10, Daniel Vetter wrote: > > On Thu, Jul 06, 2017 at 02:20:37PM +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 <peda@axentia.se> > >> --- > >> drivers/gpu/drm/drm_fb_helper.c | 232 ++++++++++++++++++++++++++++------------ > >> 1 file changed, 161 insertions(+), 71 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >> index 721511d..32d6ea1 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,140 @@ 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; > >> + 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; > >> + > >> + 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; > >> + bool replaced = false; > >> > >> - red = cmap->red; > >> - green = cmap->green; > >> - blue = cmap->blue; > >> - transp = cmap->transp; > >> - start = cmap->start; > >> + crtc = fb_helper->crtc_info[i].mode_set.crtc; > >> > >> - if (!crtc->gamma_size) { > >> - rc = -EINVAL; > >> - goto out; > >> + gamma_lut = setcmap_new_gamma_lut(crtc, cmap); > > > > Tiny nit you might want to improve (since you need to respin for my naming > > bikeshed of the property_replace_blob function anyway): Properties are > > refcounting and invariant, which means you can just create the property > > once, and then use it for all the CRTC. Slightly cleaner code. > > Yes, I thought about that, but ended up not. The reason is that as far > as I could tell, all involved crtc need not have the same original > gamma_lut. Sure, if all crtc have the same history, that should be the > case, but isn't it possible to tie things together one way first and > set some clut, then "rewire" things so that the crtc no longer have the > same history? > > But if you in the light of that still think it's wise to set the same > clut for all crtc I will factor that part out of the loop. Blob properties are invariant, if you want to change a lut you _have_ to create a new blob property. They're also reference-counted, which means users of a blob property can come&go as they wish, it will only get freed when the last one is released. So even when you change the lut of 1 CRTC the other CRTCs will be able to keep using the existing lut blob property unchanged. That's the beauty of having refcounted objects with invariant data over their lifetime, makes a lot of things a lot simpler. drm_framebuffer work the same (only their metadata is invariant, the data of the actual backing storage can change ofc, but not where that backing storage is). Allows you to do simple pointer comparison of objects to check whether their equal or something has changed. tldr; sharing blobs is perfectly safe and how this is designed to work. Cheers, Daniel
On 2017-07-12 09:03, Daniel Vetter wrote: > On Tue, Jul 11, 2017 at 02:12:26PM +0200, Peter Rosin wrote: >> On 2017-07-11 10:10, Daniel Vetter wrote: >>> Tiny nit you might want to improve (since you need to respin for my naming >>> bikeshed of the property_replace_blob function anyway): Properties are >>> refcounting and invariant, which means you can just create the property >>> once, and then use it for all the CRTC. Slightly cleaner code. >> >> Yes, I thought about that, but ended up not. The reason is that as far >> as I could tell, all involved crtc need not have the same original >> gamma_lut. Sure, if all crtc have the same history, that should be the >> case, but isn't it possible to tie things together one way first and >> set some clut, then "rewire" things so that the crtc no longer have the >> same history? >> >> But if you in the light of that still think it's wise to set the same >> clut for all crtc I will factor that part out of the loop. > > Blob properties are invariant, if you want to change a lut you _have_ to > create a new blob property. They're also reference-counted, which means > users of a blob property can come&go as they wish, it will only get freed > when the last one is released. > > So even when you change the lut of 1 CRTC the other CRTCs will be able to > keep using the existing lut blob property unchanged. That's the beauty of > having refcounted objects with invariant data over their lifetime, makes a > lot of things a lot simpler. drm_framebuffer work the same (only their > metadata is invariant, the data of the actual backing storage can change > ofc, but not where that backing storage is). Allows you to do simple > pointer comparison of objects to check whether their equal or something > has changed. > > tldr; sharing blobs is perfectly safe and how this is designed to work. Yes, I get that, but that wasn't my problem. At all. Say that you have a driver with two crtc, A and B. Then this happens: 1. A gets a clut with, say, only various red colors. 2. B gets a different clut with various green colors. 3. Someone ties things up so that one fbdev is used on both A and B. I don't know if this is possible, but if it is, the two crtc now have different cluts. 4. Via fbdev, only part of the clut is updated for this A/B combo. If A and B starts sharing clut in 4, the part that is not updated is clobbered for either crtc A or B. (updating only part of the clut is only possible with fbdev, AFAICT) Yes, it's a fringe thing to cater to... Cheers, Peter
On Wed, Jul 12, 2017 at 09:47:27AM +0200, Peter Rosin wrote: > On 2017-07-12 09:03, Daniel Vetter wrote: > > On Tue, Jul 11, 2017 at 02:12:26PM +0200, Peter Rosin wrote: > >> On 2017-07-11 10:10, Daniel Vetter wrote: > >>> Tiny nit you might want to improve (since you need to respin for my naming > >>> bikeshed of the property_replace_blob function anyway): Properties are > >>> refcounting and invariant, which means you can just create the property > >>> once, and then use it for all the CRTC. Slightly cleaner code. > >> > >> Yes, I thought about that, but ended up not. The reason is that as far > >> as I could tell, all involved crtc need not have the same original > >> gamma_lut. Sure, if all crtc have the same history, that should be the > >> case, but isn't it possible to tie things together one way first and > >> set some clut, then "rewire" things so that the crtc no longer have the > >> same history? > >> > >> But if you in the light of that still think it's wise to set the same > >> clut for all crtc I will factor that part out of the loop. > > > > Blob properties are invariant, if you want to change a lut you _have_ to > > create a new blob property. They're also reference-counted, which means > > users of a blob property can come&go as they wish, it will only get freed > > when the last one is released. > > > > So even when you change the lut of 1 CRTC the other CRTCs will be able to > > keep using the existing lut blob property unchanged. That's the beauty of > > having refcounted objects with invariant data over their lifetime, makes a > > lot of things a lot simpler. drm_framebuffer work the same (only their > > metadata is invariant, the data of the actual backing storage can change > > ofc, but not where that backing storage is). Allows you to do simple > > pointer comparison of objects to check whether their equal or something > > has changed. > > > > tldr; sharing blobs is perfectly safe and how this is designed to work. > > Yes, I get that, but that wasn't my problem. At all. > > Say that you have a driver with two crtc, A and B. Then this happens: > > 1. A gets a clut with, say, only various red colors. > 2. B gets a different clut with various green colors. > 3. Someone ties things up so that one fbdev is used on both A and B. > I don't know if this is possible, but if it is, the two crtc now > have different cluts. That's the default for fbdev on top of kms. fbdev doesn't have a concept of multi-screen. Some things are mapped to the first output only (like vblank waits). > 4. Via fbdev, only part of the clut is updated for this A/B combo. > > If A and B starts sharing clut in 4, the part that is not updated is > clobbered for either crtc A or B. > > (updating only part of the clut is only possible with fbdev, AFAICT) Meh. Like, completely meh :-) > Yes, it's a fringe thing to cater to... Yeah, let's shrug this under the table. fbdev doesn't work for multi-screen, ending up with strange behaviour is totally fine. -Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 721511d..32d6ea1 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,140 @@ 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; + 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; + + 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; + bool replaced = false; - red = cmap->red; - green = cmap->green; - blue = cmap->blue; - transp = cmap->transp; - start = cmap->start; + crtc = fb_helper->crtc_info[i].mode_set.crtc; - if (!crtc->gamma_size) { - rc = -EINVAL; - goto out; + gamma_lut = setcmap_new_gamma_lut(crtc, cmap); + if (IS_ERR(gamma_lut)) { + ret = PTR_ERR(gamma_lut); + 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)) { + drm_property_blob_put(gamma_lut); + ret = PTR_ERR(crtc_state); + goto out_state; } + drm_atomic_replace_property_blob(&crtc_state->degamma_lut, + NULL, &replaced); + drm_atomic_replace_property_blob(&crtc_state->ctm, + NULL, &replaced); + drm_atomic_replace_property_blob(&crtc_state->gamma_lut, + gamma_lut, &replaced); + crtc_state->color_mgmt_changed |= replaced; + drm_property_blob_put(gamma_lut); + } + + 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 +1368,56 @@ 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_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);
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 <peda@axentia.se> --- drivers/gpu/drm/drm_fb_helper.c | 232 ++++++++++++++++++++++++++++------------ 1 file changed, 161 insertions(+), 71 deletions(-)