| Message ID | 1499164632-5582-6-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 04, 2017 at 12:37:01PM +0200, Peter Rosin wrote: > This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get > completely obsolete. > > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/drm_fb_helper.c | 165 +++++++++++++++++++++++----------------- > 1 file changed, 94 insertions(+), 71 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index b75b1f2..7f8199a 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1257,27 +1257,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; > @@ -1310,54 +1289,68 @@ 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; > + 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; > > - drm_modeset_lock_all(dev); > - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { > - rc = setcmap_pseudo_palette(cmap, info); > - goto out; > + 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; > } > > - for (i = 0; i < fb_helper->crtc_count; i++) { > - crtc = fb_helper->crtc_info[i].mode_set.crtc; > - crtc_funcs = crtc->helper_private; > + return ret; > +} For the legacy path you need to keep the drm_modeset_lock_all (but only in setcmap_legacy). Otherwise this part here looks good. > > - red = cmap->red; > - green = cmap->green; > - blue = cmap->blue; > - transp = cmap->transp; > - start = cmap->start; > +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_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; > > - if (!crtc->gamma_size) { > - rc = -EINVAL; > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return -ENOMEM; > + drm_modeset_acquire_init(&ctx, 0); > +retry: > + ret = drm_modeset_lock_all_ctx(dev, &ctx); With atomic you don't need to grab locks, this is done behind the scenes (as long as you handle the retry/backoff correctly). See the kerneldoc for the various drm_atomic_get_*_state functions. > + if (ret) > + goto fini; > + state->acquire_ctx = &ctx; > + for (i = 0; i < fb_helper->crtc_count; i++) { > + crtc = fb_helper->crtc_info[i].mode_set.crtc; > + if (!crtc->funcs->gamma_set) { > + ret = -EINVAL; > goto out; > } > > - if (cmap->start + cmap->len > crtc->gamma_size) { > - rc = -EINVAL; > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) { > + ret = PTR_ERR(crtc_state); > goto out; > } > > @@ -1369,27 +1362,57 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); > memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b)); > > - for (j = 0; j < cmap->len; j++) { > - u16 hred, hgreen, hblue, htransp = 0xffff; > + ret = crtc->funcs->gamma_set(crtc, r, g, b, > + crtc->gamma_size, crtc_state); I guess my description of what I have in mind wasn't really clear. I think a proper atomic commit should never reuse one of the old hooks (->gamma_set) here, that's just confusing. Instead what I had in mind is to do the proper adjusting that gamma_set does here in this function, i.e. - create the new blob, fill it with the cmap data - assign that blob to the crtc state: drm_atomic_replace_property_blob(&crtc_state->gamma_lut, new_table, &temp); Note that the drm_atomic_helper_legacy_gamma_set() does that in the most convoluted way by going through a few layers. - The one thing you need to do on top is check that the gamma_lut property is supported (just check whether dev->mode_config.gamma_lut_property exists). That check is instead of checking for ->gamma_set. Checking for matching size is optional, the driver must do that already (for the atomic property). This way your previous patch isn't needed, and we don't need to change all the legacy callbacks. The only downside is that we duplicate a bit of the atomic commit setup scaffolding, but that's imo ok. You could extract that into a helper function shared between this code here and drm_atomic_helper_legacy_gamma_set(), but that seems frankly overkill to me. Creating atomic commits in the kernel is simply a bit verbose, but the benefit of the current framework is that the driver side looks a lot simpler. > + if (ret) > + goto out; > + } > > - hred = *red++; > - hgreen = *green++; > - hblue = *blue++; > + ret = drm_atomic_commit(state); > + if (ret == -EDEADLK) { > + drm_modeset_backoff(&ctx); > + goto retry; > + } > > - if (transp) > - htransp = *transp++; > +out: > + drm_modeset_drop_locks(&ctx); > +fini: > + drm_modeset_acquire_fini(&ctx); > + drm_atomic_state_put(state); > > - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info); > - if (rc) > - goto out; > - } > - if (crtc_funcs->load_lut) > - crtc_funcs->load_lut(crtc); > + return ret; > +} > + > +/** > + * 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; > + > + if (oops_in_progress) > + return -EBUSY; > + > + mutex_lock(&fb_helper->lock); > + > + if (!drm_fb_helper_is_bound(fb_helper)) { > + mutex_unlock(&fb_helper->lock); > + return -EBUSY; > } > - 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); > + > mutex_unlock(&fb_helper->lock); > - return rc; > + > + return ret; > } > EXPORT_SYMBOL(drm_fb_helper_setcmap); Besides the 2 comments this looks good and will get my r-b once revised. Also on patches 1-3: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Daniel > > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2017-07-05 08:21, Daniel Vetter wrote: > On Tue, Jul 04, 2017 at 12:37:01PM +0200, Peter Rosin wrote: >> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get >> completely obsolete. >> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 165 +++++++++++++++++++++++----------------- >> 1 file changed, 94 insertions(+), 71 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index b75b1f2..7f8199a 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1257,27 +1257,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; >> @@ -1310,54 +1289,68 @@ 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; >> + 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; >> >> - drm_modeset_lock_all(dev); >> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { >> - rc = setcmap_pseudo_palette(cmap, info); >> - goto out; >> + 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; >> } >> >> - for (i = 0; i < fb_helper->crtc_count; i++) { >> - crtc = fb_helper->crtc_info[i].mode_set.crtc; >> - crtc_funcs = crtc->helper_private; >> + return ret; >> +} > > For the legacy path you need to keep the drm_modeset_lock_all (but only in > setcmap_legacy). Otherwise this part here looks good. Oops, didn't intend to zap that one. Thanks for catching! >> >> - red = cmap->red; >> - green = cmap->green; >> - blue = cmap->blue; >> - transp = cmap->transp; >> - start = cmap->start; >> +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_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; >> >> - if (!crtc->gamma_size) { >> - rc = -EINVAL; >> + state = drm_atomic_state_alloc(dev); >> + if (!state) >> + return -ENOMEM; >> + drm_modeset_acquire_init(&ctx, 0); >> +retry: >> + ret = drm_modeset_lock_all_ctx(dev, &ctx); > > With atomic you don't need to grab locks, this is done behind the scenes > (as long as you handle the retry/backoff correctly). See the kerneldoc for > the various drm_atomic_get_*_state functions. It doesn't work if I remove it. What is the disconnect? >> + if (ret) >> + goto fini; >> + state->acquire_ctx = &ctx; >> + for (i = 0; i < fb_helper->crtc_count; i++) { >> + crtc = fb_helper->crtc_info[i].mode_set.crtc; >> + if (!crtc->funcs->gamma_set) { >> + ret = -EINVAL; >> goto out; >> } >> >> - if (cmap->start + cmap->len > crtc->gamma_size) { >> - rc = -EINVAL; >> + crtc_state = drm_atomic_get_crtc_state(state, crtc); >> + if (IS_ERR(crtc_state)) { >> + ret = PTR_ERR(crtc_state); >> goto out; >> } >> >> @@ -1369,27 +1362,57 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) >> memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); >> memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b)); >> >> - for (j = 0; j < cmap->len; j++) { >> - u16 hred, hgreen, hblue, htransp = 0xffff; >> + ret = crtc->funcs->gamma_set(crtc, r, g, b, >> + crtc->gamma_size, crtc_state); > > I guess my description of what I have in mind wasn't really clear. I think > a proper atomic commit should never reuse one of the old hooks > (->gamma_set) here, that's just confusing. Instead what I had in mind is > to do the proper adjusting that gamma_set does here in this function, i.e. > > - create the new blob, fill it with the cmap data > > - assign that blob to the crtc state: > > drm_atomic_replace_property_blob(&crtc_state->gamma_lut, > new_table, &temp); That function is static, and... > Note that the drm_atomic_helper_legacy_gamma_set() does that in the most > convoluted way by going through a few layers. > > - The one thing you need to do on top is check that the gamma_lut property > is supported (just check whether dev->mode_config.gamma_lut_property > exists). That check is instead of checking for ->gamma_set. ...drm_atomic_helper_legacy_gamma_set calls drm_atomic_crtc_set_property which is already exported and performs all the checks I can think of... > Checking for matching size is optional, the driver must do that already > (for the atomic property). ...except this one. > This way your previous patch isn't needed, and we don't need to change all > the legacy callbacks. The only downside is that we duplicate a bit of the > atomic commit setup scaffolding, but that's imo ok. You could extract that > into a helper function shared between this code here and > drm_atomic_helper_legacy_gamma_set(), but that seems frankly overkill to > me. Creating atomic commits in the kernel is simply a bit verbose, but the > benefit of the current framework is that the driver side looks a lot > simpler. But, yup, ditching 4/16 feels very nice. I'll mimic the code in drm_atomic_helper_legacy_gamma_set for the v4 version of setcmap_atomic. Good suggestion, and thanks for the more verbose explanation. >> + if (ret) >> + goto out; >> + } >> >> - hred = *red++; >> - hgreen = *green++; >> - hblue = *blue++; >> + ret = drm_atomic_commit(state); >> + if (ret == -EDEADLK) { >> + drm_modeset_backoff(&ctx); >> + goto retry; >> + } >> >> - if (transp) >> - htransp = *transp++; >> +out: >> + drm_modeset_drop_locks(&ctx); >> +fini: >> + drm_modeset_acquire_fini(&ctx); >> + drm_atomic_state_put(state); >> >> - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info); >> - if (rc) >> - goto out; >> - } >> - if (crtc_funcs->load_lut) >> - crtc_funcs->load_lut(crtc); >> + return ret; >> +} >> + >> +/** >> + * 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; >> + >> + if (oops_in_progress) >> + return -EBUSY; >> + >> + mutex_lock(&fb_helper->lock); >> + >> + if (!drm_fb_helper_is_bound(fb_helper)) { >> + mutex_unlock(&fb_helper->lock); >> + return -EBUSY; >> } >> - 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); >> + >> mutex_unlock(&fb_helper->lock); >> - return rc; >> + >> + return ret; >> } >> EXPORT_SYMBOL(drm_fb_helper_setcmap); > > Besides the 2 comments this looks good and will get my r-b once revised. > > Also on patches 1-3: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Excellent! Cheers, peda > Cheers, Daniel >> >> -- >> 2.1.4 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Wed, Jul 5, 2017 at 7:50 PM, Peter Rosin <peda@axentia.se> wrote: >>> +retry: >>> + ret = drm_modeset_lock_all_ctx(dev, &ctx); >> >> With atomic you don't need to grab locks, this is done behind the scenes >> (as long as you handle the retry/backoff correctly). See the kerneldoc for >> the various drm_atomic_get_*_state functions. > > It doesn't work if I remove it. What is the disconnect? Good question, didn't spot this at first, but your backoff/retry logic is proken. When typing drm_modeset_lock locking code please make sure you've enabled both CONFIG_PROVE_LOCKING and CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Without these two it's really easy to get this wrong. Please also read https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-locking carefully plus all the kernel-doc of the various hooks. This stuff is a really tricky locking scheme, it takes a while to understand it and implement it correctly. Which is why all the locking magic is in shared code and for normal drivers no need think about it. For the fundamental algorithm, you can also check out the docs for w/w mutexes at https://www.kernel.org/doc/Documentation/locking/ww-mutex-design.txt Might also help to read a bunch of the other locking paths again, with my patches there's a few just in drm_fbdev_helper.c. I'll leave you with these snippets here since I think this is fun to learn, but when you're stuck I'm happy to help learn. -Daniel
On 2017-07-06 07:55, Daniel Vetter wrote: > On Wed, Jul 5, 2017 at 7:50 PM, Peter Rosin <peda@axentia.se> wrote: >>>> +retry: >>>> + ret = drm_modeset_lock_all_ctx(dev, &ctx); >>> >>> With atomic you don't need to grab locks, this is done behind the scenes >>> (as long as you handle the retry/backoff correctly). See the kerneldoc for >>> the various drm_atomic_get_*_state functions. >> >> It doesn't work if I remove it. What is the disconnect? > > Good question, Duh, for symmetry I also removed the dropping of the locks. So the next call of course had no chance of getting access. How silly of me... > didn't spot this at first, but your backoff/retry logic > is proken. When typing drm_modeset_lock locking code please make sure > you've enabled both CONFIG_PROVE_LOCKING and > CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Without these two it's really easy to > get this wrong. Please also read > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-locking > carefully plus all the kernel-doc of the various hooks. This stuff is > a really tricky locking scheme, it takes a while to understand it and > implement it correctly. Which is why all the locking magic is in > shared code and for normal drivers no need think about it. For the > fundamental algorithm, you can also check out the docs for w/w mutexes > at https://www.kernel.org/doc/Documentation/locking/ww-mutex-design.txt > > Might also help to read a bunch of the other locking paths again, with > my patches there's a few just in drm_fbdev_helper.c. I'll leave you > with these snippets here since I think this is fun to learn, but when > you're stuck I'm happy to help learn. I'll take a long look at this before I send a cleaned up v4. Thanks for the pointers... Cheers, peda
> >> @@ -1369,27 +1362,57 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > >> memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); > >> memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b)); > >> > >> - for (j = 0; j < cmap->len; j++) { > >> - u16 hred, hgreen, hblue, htransp = 0xffff; > >> + ret = crtc->funcs->gamma_set(crtc, r, g, b, > >> + crtc->gamma_size, crtc_state); > > > > I guess my description of what I have in mind wasn't really clear. I think > > a proper atomic commit should never reuse one of the old hooks > > (->gamma_set) here, that's just confusing. Instead what I had in mind is > > to do the proper adjusting that gamma_set does here in this function, i.e. > > > > - create the new blob, fill it with the cmap data > > > > - assign that blob to the crtc state: > > > > drm_atomic_replace_property_blob(&crtc_state->gamma_lut, > > new_table, &temp); > > That function is static, and... Missed these comments here. I think we should just make them unstatic, that might also explain why gamma_set is going this indirect path. Means we need some kerneldoc, but that's not much work. Ping the original author if you have questions. Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index b75b1f2..7f8199a 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1257,27 +1257,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; @@ -1310,54 +1289,68 @@ 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; + 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; - drm_modeset_lock_all(dev); - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { - rc = setcmap_pseudo_palette(cmap, info); - goto out; + 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; } - for (i = 0; i < fb_helper->crtc_count; i++) { - crtc = fb_helper->crtc_info[i].mode_set.crtc; - crtc_funcs = crtc->helper_private; + return ret; +} - red = cmap->red; - green = cmap->green; - blue = cmap->blue; - transp = cmap->transp; - start = cmap->start; +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_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; - if (!crtc->gamma_size) { - rc = -EINVAL; + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto fini; + state->acquire_ctx = &ctx; + for (i = 0; i < fb_helper->crtc_count; i++) { + crtc = fb_helper->crtc_info[i].mode_set.crtc; + if (!crtc->funcs->gamma_set) { + ret = -EINVAL; goto out; } - if (cmap->start + cmap->len > crtc->gamma_size) { - rc = -EINVAL; + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) { + ret = PTR_ERR(crtc_state); goto out; } @@ -1369,27 +1362,57 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b)); - for (j = 0; j < cmap->len; j++) { - u16 hred, hgreen, hblue, htransp = 0xffff; + ret = crtc->funcs->gamma_set(crtc, r, g, b, + crtc->gamma_size, crtc_state); + if (ret) + goto out; + } - hred = *red++; - hgreen = *green++; - hblue = *blue++; + ret = drm_atomic_commit(state); + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } - if (transp) - htransp = *transp++; +out: + drm_modeset_drop_locks(&ctx); +fini: + drm_modeset_acquire_fini(&ctx); + drm_atomic_state_put(state); - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info); - if (rc) - goto out; - } - if (crtc_funcs->load_lut) - crtc_funcs->load_lut(crtc); + return ret; +} + +/** + * 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; + + if (oops_in_progress) + return -EBUSY; + + mutex_lock(&fb_helper->lock); + + if (!drm_fb_helper_is_bound(fb_helper)) { + mutex_unlock(&fb_helper->lock); + return -EBUSY; } - 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); + mutex_unlock(&fb_helper->lock); - return rc; + + return ret; } EXPORT_SYMBOL(drm_fb_helper_setcmap);
This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get completely obsolete. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/drm_fb_helper.c | 165 +++++++++++++++++++++++----------------- 1 file changed, 94 insertions(+), 71 deletions(-)