| Message ID | 1497986735-14418-2-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: > This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get > totally obsolete. > > I think the gamma_store can end up invalid on error. But the way I read > it, that can happen in drm_mode_gamma_set_ioctl as well, so why should > this pesky legacy fbdev stuff be any better? > > drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, > it saves it to the gamma_store which should already be up to date with what > .gamma_get would return and is thus a nop. So, zap it. Removing drm_fb_helper_save_lut_atomic should be a separate patch I think. > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/drm_fb_helper.c | 131 ++++++++++++---------------------------- > 1 file changed, 40 insertions(+), 91 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 574af01..cc2d55d 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -229,22 +229,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, > } > EXPORT_SYMBOL(drm_fb_helper_remove_one_connector); > > -static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper) > -{ > - uint16_t *r_base, *g_base, *b_base; > - int i; > - > - if (helper->funcs->gamma_get == NULL) > - return; > - > - r_base = crtc->gamma_store; > - g_base = r_base + crtc->gamma_size; > - b_base = g_base + crtc->gamma_size; > - > - for (i = 0; i < crtc->gamma_size; i++) > - helper->funcs->gamma_get(crtc, &r_base[i], &g_base[i], &b_base[i], i); > -} > - > static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) > { > uint16_t *r_base, *g_base, *b_base; > @@ -285,7 +269,6 @@ int drm_fb_helper_debug_enter(struct fb_info *info) > if (drm_drv_uses_atomic_modeset(mode_set->crtc->dev)) > continue; > > - drm_fb_helper_save_lut_atomic(mode_set->crtc, helper); > funcs->mode_set_base_atomic(mode_set->crtc, > mode_set->fb, > mode_set->x, > @@ -1167,50 +1150,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; > - > - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { This case here seems gone, and it was also the pièce de résistance when I tried tackling fbdev lut support. As far as I understand this stuff we do not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is pointless. But I'm honestly not really clear. I think removing this case should also be a separate patch, and I'd very much prefer that someone with some fbdev-clue would ack it. > - u32 *palette; > - u32 value; > - /* place color in psuedopalette */ > - if (regno > 16) > - return -EINVAL; > - palette = (u32 *)info->pseudo_palette; > - red >>= (16 - info->var.red.length); > - green >>= (16 - info->var.green.length); > - blue >>= (16 - info->var.blue.length); > - value = (red << info->var.red.offset) | > - (green << info->var.green.offset) | > - (blue << info->var.blue.offset); > - if (info->var.transp.length > 0) { > - u32 mask = (1 << info->var.transp.length) - 1; > - > - mask <<= info->var.transp.offset; > - value |= mask; > - } > - palette[regno] = value; > - return 0; > - } > - > - /* > - * 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; > -} > - > /** > * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap > * @cmap: cmap to set > @@ -1220,51 +1159,61 @@ int drm_fb_helper_setcmap(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_modeset_acquire_ctx ctx; > struct drm_crtc *crtc; > - int i, j, rc = 0; > - int start; > + u16 *r, *g, *b; > + int i, ret; > > if (oops_in_progress) > return -EBUSY; > > - drm_modeset_lock_all(dev); > + if (cmap->start + cmap->len < cmap->start) > + return -EINVAL; > + > + drm_modeset_acquire_init(&ctx, 0); > +retry: > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > + if (ret) > + goto out; > if (!drm_fb_helper_is_bound(fb_helper)) { > - drm_modeset_unlock_all(dev); > - return -EBUSY; > + ret = -EBUSY; > + goto out; > } > > 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->funcs->gamma_set || !crtc->gamma_size) { > + ret = -EINVAL; > + goto out; > + } > > - for (j = 0; j < cmap->len; j++) { > - u16 hred, hgreen, hblue, htransp = 0xffff; > + if (cmap->start + cmap->len > crtc->gamma_size) { > + ret = -EINVAL; > + goto out; > + } > > - hred = *red++; > - hgreen = *green++; > - hblue = *blue++; > + r = crtc->gamma_store; > + g = r + crtc->gamma_size; > + b = g + crtc->gamma_size; > > - if (transp) > - htransp = *transp++; > + memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(u16)); > + memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(u16)); > + memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(u16)); > > - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info); > - if (rc) > - goto out; > - } > - if (crtc_funcs->load_lut) > - crtc_funcs->load_lut(crtc); > + ret = crtc->funcs->gamma_set(crtc, r, g, b, > + crtc->gamma_size, &ctx); > + if (ret) > + break; > } > - out: > - drm_modeset_unlock_all(dev); > - return rc; > +out: > + if (ret == -EDEADLK) { > + drm_modeset_backoff(&ctx); > + goto retry; > + } > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > + return ret; It's a pre-existing bug, but should we also try to restore the fbdev lut in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug, but might be relevant for your use-case. Just try to run both an fbdev application and some kms-native thing, and then SIGKILL the native kms app. But since pre-existing not really required, and probably too much effort. Thanks, Daniel > } > 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 21/06/17 04:38 PM, Daniel Vetter wrote: > On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: >> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get >> totally obsolete. >> >> I think the gamma_store can end up invalid on error. But the way I read >> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should >> this pesky legacy fbdev stuff be any better? >> >> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, >> it saves it to the gamma_store which should already be up to date with what >> .gamma_get would return and is thus a nop. So, zap it. > > Removing drm_fb_helper_save_lut_atomic should be a separate patch I > think. > >> Signed-off-by: Peter Rosin <peda@axentia.se> [...] >> @@ -1167,50 +1150,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; >> - >> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { > > This case here seems gone, and it was also the pièce de résistance when I > tried tackling fbdev lut support. As far as I understand this stuff we do > not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is > pointless. But I'm honestly not really clear. > > I think removing this case should also be a separate patch, and I'd very > much prefer that someone with some fbdev-clue would ack it. No need for anyone with fbdev-clue, just run fbset -i. :) fbcon uses a TRUECOLOR visual at depths > 8. > It's a pre-existing bug, but should we also try to restore the fbdev lut > in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug, > but might be relevant for your use-case. Just try to run both an fbdev > application and some kms-native thing, and then SIGKILL the native kms > app. > > But since pre-existing not really required, and probably too much effort. I suspect something like that indeed needs to be done though. E.g. killing Xorg results in fbcon continuing to use the LUT set by Xorg.
On Wed, Jun 21, 2017 at 04:59:31PM +0900, Michel Dänzer wrote: > On 21/06/17 04:38 PM, Daniel Vetter wrote: > > On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: > >> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get > >> totally obsolete. > >> > >> I think the gamma_store can end up invalid on error. But the way I read > >> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should > >> this pesky legacy fbdev stuff be any better? > >> > >> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, > >> it saves it to the gamma_store which should already be up to date with what > >> .gamma_get would return and is thus a nop. So, zap it. > > > > Removing drm_fb_helper_save_lut_atomic should be a separate patch I > > think. > > > >> Signed-off-by: Peter Rosin <peda@axentia.se> > > [...] > > >> @@ -1167,50 +1150,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; > >> - > >> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { > > > > This case here seems gone, and it was also the pièce de résistance when I > > tried tackling fbdev lut support. As far as I understand this stuff we do > > not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is > > pointless. But I'm honestly not really clear. > > > > I think removing this case should also be a separate patch, and I'd very > > much prefer that someone with some fbdev-clue would ack it. > > No need for anyone with fbdev-clue, just run fbset -i. :) fbcon uses a > TRUECOLOR visual at depths > 8. Then I understand even less what exactly this code does ... Should we keep it? Is it just pure cargo-cult? Only needed when we'd change the color depth of the fbdev buffer, which we never do at runtime? > > It's a pre-existing bug, but should we also try to restore the fbdev lut > > in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug, > > but might be relevant for your use-case. Just try to run both an fbdev > > application and some kms-native thing, and then SIGKILL the native kms > > app. > > > > But since pre-existing not really required, and probably too much effort. > > I suspect something like that indeed needs to be done though. E.g. > killing Xorg results in fbcon continuing to use the LUT set by Xorg. Ok, the only trouble with that is atomic kms locking, which requires that we can only do 1 commit per lock-holding time, and also drm_modeset_lock_all is an evil hack and needs to be phased out. Two solutions: - We just update the lut after we've dropped the locks again in restore_fbdev_mode. - We need both a legacy path, in restore_fbdev_mode_legacy, and an atomic path in restore_fbdev_mode_atomic. The latter cannot use the gamme_set callback, since that would call drm_atomic_helper_legacy_gamma_set for atomic drivers, which would result in two calls to drm_atomic_commit in one critical sections. Instead that needs to open-code the logic in drm_atomic_helper_legacy_gamma_set and integrate it into the overall fbdev restore commit. The 2nd option is a bit more typing, but much cleaner. It also fits better into some of the refactoring plans I have (I want to get rid of drm_modeset_lock_all in the fbdev emulation). And has the benefit of giving drivers a bit more complex fbdev emulation atomic commits, which would be good for testing I think. Cheers, Daniel
On 21/06/17 05:14 PM, Daniel Vetter wrote: > On Wed, Jun 21, 2017 at 04:59:31PM +0900, Michel Dänzer wrote: >> On 21/06/17 04:38 PM, Daniel Vetter wrote: >>> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: >>>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get >>>> totally obsolete. >>>> >>>> I think the gamma_store can end up invalid on error. But the way I read >>>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should >>>> this pesky legacy fbdev stuff be any better? >>>> >>>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, >>>> it saves it to the gamma_store which should already be up to date with what >>>> .gamma_get would return and is thus a nop. So, zap it. >>> >>> Removing drm_fb_helper_save_lut_atomic should be a separate patch I >>> think. >>> >>>> Signed-off-by: Peter Rosin <peda@axentia.se> >> >> [...] >> >>>> @@ -1167,50 +1150,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; >>>> - >>>> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { >>> >>> This case here seems gone, and it was also the pièce de résistance when I >>> tried tackling fbdev lut support. As far as I understand this stuff we do >>> not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is >>> pointless. But I'm honestly not really clear. >>> >>> I think removing this case should also be a separate patch, and I'd very >>> much prefer that someone with some fbdev-clue would ack it. >> >> No need for anyone with fbdev-clue, just run fbset -i. :) Adding Bartlomiej and the linux-fbdev list, just in case I'm wrong below. >> fbcon uses a TRUECOLOR visual at depths > 8. > > Then I understand even less what exactly this code does ... Should we keep > it? I think we need it. It updates the entries of info->pseudo_palette, which is kind of a pseudocolor palette mapping 16 indices to pixel values to write to the framebuffer. If the setcolreg hook is removed, the setcmap hook needs to do this instead.
On 2017-06-21 09:38, Daniel Vetter wrote: > On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: >> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get >> totally obsolete. >> >> I think the gamma_store can end up invalid on error. But the way I read >> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should >> this pesky legacy fbdev stuff be any better? >> >> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, >> it saves it to the gamma_store which should already be up to date with what >> .gamma_get would return and is thus a nop. So, zap it. > > Removing drm_fb_helper_save_lut_atomic should be a separate patch I > think. Then 3 patches would be needed, first some hybrid thing that does it the old way, but also stores the lut in .gamma_store, then the split-out that removes drm_fb_helper_save_lut_atomic, then whatever is needed to get to the desired code. I can certainly do that, but do you want me to? I.e., the statement that drm_fb_helper_save_lut_atomic is a nop is only true when (part of) the other patch is also considered. >> Signed-off-by: Peter Rosin <peda@axentia.se> > >> --- >> drivers/gpu/drm/drm_fb_helper.c | 131 ++++++++++++---------------------------- >> 1 file changed, 40 insertions(+), 91 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 574af01..cc2d55d 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -229,22 +229,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, >> } >> EXPORT_SYMBOL(drm_fb_helper_remove_one_connector); >> >> -static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper) >> -{ >> - uint16_t *r_base, *g_base, *b_base; >> - int i; >> - >> - if (helper->funcs->gamma_get == NULL) >> - return; >> - >> - r_base = crtc->gamma_store; >> - g_base = r_base + crtc->gamma_size; >> - b_base = g_base + crtc->gamma_size; >> - >> - for (i = 0; i < crtc->gamma_size; i++) >> - helper->funcs->gamma_get(crtc, &r_base[i], &g_base[i], &b_base[i], i); >> -} >> - >> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) >> { >> uint16_t *r_base, *g_base, *b_base; >> @@ -285,7 +269,6 @@ int drm_fb_helper_debug_enter(struct fb_info *info) >> if (drm_drv_uses_atomic_modeset(mode_set->crtc->dev)) >> continue; >> >> - drm_fb_helper_save_lut_atomic(mode_set->crtc, helper); >> funcs->mode_set_base_atomic(mode_set->crtc, >> mode_set->fb, >> mode_set->x, >> @@ -1167,50 +1150,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; >> - >> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { > > This case here seems gone, and it was also the pièce de résistance when I > tried tackling fbdev lut support. As far as I understand this stuff we do > not support FB_VISUAL_TRUECOLOR palette, and all that bitshifting here is > pointless. But I'm honestly not really clear. Oops, sorry, I simply missed that, I'll have a closer look... > I think removing this case should also be a separate patch, and I'd very > much prefer that someone with some fbdev-clue would ack it. > >> - u32 *palette; >> - u32 value; >> - /* place color in psuedopalette */ >> - if (regno > 16) >> - return -EINVAL; >> - palette = (u32 *)info->pseudo_palette; >> - red >>= (16 - info->var.red.length); >> - green >>= (16 - info->var.green.length); >> - blue >>= (16 - info->var.blue.length); >> - value = (red << info->var.red.offset) | >> - (green << info->var.green.offset) | >> - (blue << info->var.blue.offset); >> - if (info->var.transp.length > 0) { >> - u32 mask = (1 << info->var.transp.length) - 1; >> - >> - mask <<= info->var.transp.offset; >> - value |= mask; >> - } >> - palette[regno] = value; >> - return 0; >> - } >> - >> - /* >> - * 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; >> -} >> - >> /** >> * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap >> * @cmap: cmap to set >> @@ -1220,51 +1159,61 @@ int drm_fb_helper_setcmap(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_modeset_acquire_ctx ctx; >> struct drm_crtc *crtc; >> - int i, j, rc = 0; >> - int start; >> + u16 *r, *g, *b; >> + int i, ret; >> >> if (oops_in_progress) >> return -EBUSY; >> >> - drm_modeset_lock_all(dev); >> + if (cmap->start + cmap->len < cmap->start) >> + return -EINVAL; >> + >> + drm_modeset_acquire_init(&ctx, 0); >> +retry: >> + ret = drm_modeset_lock_all_ctx(dev, &ctx); >> + if (ret) >> + goto out; >> if (!drm_fb_helper_is_bound(fb_helper)) { >> - drm_modeset_unlock_all(dev); >> - return -EBUSY; >> + ret = -EBUSY; >> + goto out; >> } >> >> 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->funcs->gamma_set || !crtc->gamma_size) { >> + ret = -EINVAL; >> + goto out; >> + } >> >> - for (j = 0; j < cmap->len; j++) { >> - u16 hred, hgreen, hblue, htransp = 0xffff; >> + if (cmap->start + cmap->len > crtc->gamma_size) { >> + ret = -EINVAL; >> + goto out; >> + } >> >> - hred = *red++; >> - hgreen = *green++; >> - hblue = *blue++; >> + r = crtc->gamma_store; >> + g = r + crtc->gamma_size; >> + b = g + crtc->gamma_size; >> >> - if (transp) >> - htransp = *transp++; >> + memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(u16)); >> + memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(u16)); >> + memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(u16)); >> >> - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info); >> - if (rc) >> - goto out; >> - } >> - if (crtc_funcs->load_lut) >> - crtc_funcs->load_lut(crtc); >> + ret = crtc->funcs->gamma_set(crtc, r, g, b, >> + crtc->gamma_size, &ctx); >> + if (ret) >> + break; >> } >> - out: >> - drm_modeset_unlock_all(dev); >> - return rc; >> +out: >> + if (ret == -EDEADLK) { >> + drm_modeset_backoff(&ctx); >> + goto retry; >> + } >> + drm_modeset_drop_locks(&ctx); >> + drm_modeset_acquire_fini(&ctx); >> + >> + return ret; > > It's a pre-existing bug, but should we also try to restore the fbdev lut > in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug, > but might be relevant for your use-case. Just try to run both an fbdev > application and some kms-native thing, and then SIGKILL the native kms > app. > > But since pre-existing not really required, and probably too much effort. Good thing too, because I don't really know my way around this code... Cheers, peda > Thanks, Daniel > >> } >> 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 Wed, Jun 21, 2017 at 11:40:52AM +0200, Peter Rosin wrote: > On 2017-06-21 09:38, Daniel Vetter wrote: > > On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: > >> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get > >> totally obsolete. > >> > >> I think the gamma_store can end up invalid on error. But the way I read > >> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should > >> this pesky legacy fbdev stuff be any better? > >> > >> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, > >> it saves it to the gamma_store which should already be up to date with what > >> .gamma_get would return and is thus a nop. So, zap it. > > > > Removing drm_fb_helper_save_lut_atomic should be a separate patch I > > think. > > Then 3 patches would be needed, first some hybrid thing that does it the > old way, but also stores the lut in .gamma_store, then the split-out that > removes drm_fb_helper_save_lut_atomic, then whatever is needed to get > to the desired code. I can certainly do that, but do you want me to? Explain that in the commit message and it's fine. > > It's a pre-existing bug, but should we also try to restore the fbdev lut > > in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug, > > but might be relevant for your use-case. Just try to run both an fbdev > > application and some kms-native thing, and then SIGKILL the native kms > > app. > > > > But since pre-existing not really required, and probably too much effort. > > Good thing too, because I don't really know my way around this code... Btw I cc'ed you on one of my patches in the fbdev locking series, we might need to do the same legacy vs. atomic split for the new lut code as I did for dpms. The rule with atomic is that you can't do multiple commits under drm_modeset_lock_all, you either have to do one overall atomic commit (preferred) or drop&reacquire locks again. This matters for LUT since you're updating the LUT on all CRTCs, which when using the gamma_set atomic helper would be multiple commits :-/ Using the dpms patch as template it shouldn't be too hard to address that for your patch here too. -Daniel
On 2017-06-22 08:36, Daniel Vetter wrote: > On Wed, Jun 21, 2017 at 11:40:52AM +0200, Peter Rosin wrote: >> On 2017-06-21 09:38, Daniel Vetter wrote: >>> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: >>>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get >>>> totally obsolete. >>>> >>>> I think the gamma_store can end up invalid on error. But the way I read >>>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should >>>> this pesky legacy fbdev stuff be any better? >>>> >>>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, >>>> it saves it to the gamma_store which should already be up to date with what >>>> .gamma_get would return and is thus a nop. So, zap it. >>> >>> Removing drm_fb_helper_save_lut_atomic should be a separate patch I >>> think. >> >> Then 3 patches would be needed, first some hybrid thing that does it the >> old way, but also stores the lut in .gamma_store, then the split-out that >> removes drm_fb_helper_save_lut_atomic, then whatever is needed to get >> to the desired code. I can certainly do that, but do you want me to? > > Explain that in the commit message and it's fine. I did the split in v2, I assume that's ok too. Better in case anyone ever needs to run a bisect on this... >>> It's a pre-existing bug, but should we also try to restore the fbdev lut >>> in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug, >>> but might be relevant for your use-case. Just try to run both an fbdev >>> application and some kms-native thing, and then SIGKILL the native kms >>> app. >>> >>> But since pre-existing not really required, and probably too much effort. >> >> Good thing too, because I don't really know my way around this code... > > Btw I cc'ed you on one of my patches in the fbdev locking series, we might > need to do the same legacy vs. atomic split for the new lut code as I did > for dpms. The rule with atomic is that you can't do multiple commits under > drm_modeset_lock_all, you either have to do one overall atomic commit > (preferred) or drop&reacquire locks again. This matters for LUT since > you're updating the LUT on all CRTCs, which when using the gamma_set > atomic helper would be multiple commits :-/ Ahh, ok, I see the problem. > Using the dpms patch as template it shouldn't be too hard to address that > for your patch here too. Hmm, in that patch you handle the legacy case in a separate function, and doing that for the lut case looks difficult when the atomic commit happens inside the helper (typically drm_atomic_helper_legacy_gamma_set which could perhaps be handled, but a real drag to handle for drivers that have a custom crtc .gamma_set). So, I'm aiming for the drop&reacquire approach... However, I don't have all of that series, and I suspect that is why I do not have any fb_helper->lock. I'll send my best guess as a follow-up to patch 3/14 in v2. Cheers, peda
On Thu, Jun 22, 2017 at 10:48:10AM +0200, Peter Rosin wrote: > On 2017-06-22 08:36, Daniel Vetter wrote: > > On Wed, Jun 21, 2017 at 11:40:52AM +0200, Peter Rosin wrote: > >> On 2017-06-21 09:38, Daniel Vetter wrote: > >>> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote: > >>>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get > >>>> totally obsolete. > >>>> > >>>> I think the gamma_store can end up invalid on error. But the way I read > >>>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should > >>>> this pesky legacy fbdev stuff be any better? > >>>> > >>>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, > >>>> it saves it to the gamma_store which should already be up to date with what > >>>> .gamma_get would return and is thus a nop. So, zap it. > >>> > >>> Removing drm_fb_helper_save_lut_atomic should be a separate patch I > >>> think. > >> > >> Then 3 patches would be needed, first some hybrid thing that does it the > >> old way, but also stores the lut in .gamma_store, then the split-out that > >> removes drm_fb_helper_save_lut_atomic, then whatever is needed to get > >> to the desired code. I can certainly do that, but do you want me to? > > > > Explain that in the commit message and it's fine. > > I did the split in v2, I assume that's ok too. Better in case anyone ever > needs to run a bisect on this... > > >>> It's a pre-existing bug, but should we also try to restore the fbdev lut > >>> in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug, > >>> but might be relevant for your use-case. Just try to run both an fbdev > >>> application and some kms-native thing, and then SIGKILL the native kms > >>> app. > >>> > >>> But since pre-existing not really required, and probably too much effort. > >> > >> Good thing too, because I don't really know my way around this code... > > > > Btw I cc'ed you on one of my patches in the fbdev locking series, we might > > need to do the same legacy vs. atomic split for the new lut code as I did > > for dpms. The rule with atomic is that you can't do multiple commits under > > drm_modeset_lock_all, you either have to do one overall atomic commit > > (preferred) or drop&reacquire locks again. This matters for LUT since > > you're updating the LUT on all CRTCs, which when using the gamma_set > > atomic helper would be multiple commits :-/ > > Ahh, ok, I see the problem. > > > Using the dpms patch as template it shouldn't be too hard to address that > > for your patch here too. > > Hmm, in that patch you handle the legacy case in a separate function, and > doing that for the lut case looks difficult when the atomic commit happens > inside the helper (typically drm_atomic_helper_legacy_gamma_set which > could perhaps be handled, but a real drag to handle for drivers that have > a custom crtc .gamma_set). Yeah, that's essentially why you need 2 functions. Legacy one calls the legacy interfaces, the atomic one calls the new fancy atomic property stuff directly (i.e. it open-codes the property setting dance from the helper, but with one atomic commit across all crtc). The reason that the legacy callback functions are helpers and not just default behavior is that I expected some drivers to need special hacks to keep their existing legacy kms userspace working. Atomic helpers unified behaviour a lot that beforehand was slightly inconsistent. I somewhat expect that long-term we'll unexport all the compat helpers and just make them the default for all atomic drivers. > So, I'm aiming for the drop&reacquire approach... Hm, I prefer the open-coding, that's at least what we do everywhere else. Has the benefit that it's more atomic (one commit for everything). > However, I don't have all of that series, and I suspect that is why I do > not have any fb_helper->lock. Oh sorry, entire series is on dri-devel. I can do a git branch too if you need one, atm it's in my general pile: https://cgit.freedesktop.org/~danvet/drm/log/ Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 574af01..cc2d55d 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -229,22 +229,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, } EXPORT_SYMBOL(drm_fb_helper_remove_one_connector); -static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper) -{ - uint16_t *r_base, *g_base, *b_base; - int i; - - if (helper->funcs->gamma_get == NULL) - return; - - r_base = crtc->gamma_store; - g_base = r_base + crtc->gamma_size; - b_base = g_base + crtc->gamma_size; - - for (i = 0; i < crtc->gamma_size; i++) - helper->funcs->gamma_get(crtc, &r_base[i], &g_base[i], &b_base[i], i); -} - static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc) { uint16_t *r_base, *g_base, *b_base; @@ -285,7 +269,6 @@ int drm_fb_helper_debug_enter(struct fb_info *info) if (drm_drv_uses_atomic_modeset(mode_set->crtc->dev)) continue; - drm_fb_helper_save_lut_atomic(mode_set->crtc, helper); funcs->mode_set_base_atomic(mode_set->crtc, mode_set->fb, mode_set->x, @@ -1167,50 +1150,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; - - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { - u32 *palette; - u32 value; - /* place color in psuedopalette */ - if (regno > 16) - return -EINVAL; - palette = (u32 *)info->pseudo_palette; - red >>= (16 - info->var.red.length); - green >>= (16 - info->var.green.length); - blue >>= (16 - info->var.blue.length); - value = (red << info->var.red.offset) | - (green << info->var.green.offset) | - (blue << info->var.blue.offset); - if (info->var.transp.length > 0) { - u32 mask = (1 << info->var.transp.length) - 1; - - mask <<= info->var.transp.offset; - value |= mask; - } - palette[regno] = value; - return 0; - } - - /* - * 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; -} - /** * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap * @cmap: cmap to set @@ -1220,51 +1159,61 @@ int drm_fb_helper_setcmap(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_modeset_acquire_ctx ctx; struct drm_crtc *crtc; - int i, j, rc = 0; - int start; + u16 *r, *g, *b; + int i, ret; if (oops_in_progress) return -EBUSY; - drm_modeset_lock_all(dev); + if (cmap->start + cmap->len < cmap->start) + return -EINVAL; + + drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto out; if (!drm_fb_helper_is_bound(fb_helper)) { - drm_modeset_unlock_all(dev); - return -EBUSY; + ret = -EBUSY; + goto out; } 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->funcs->gamma_set || !crtc->gamma_size) { + ret = -EINVAL; + goto out; + } - for (j = 0; j < cmap->len; j++) { - u16 hred, hgreen, hblue, htransp = 0xffff; + if (cmap->start + cmap->len > crtc->gamma_size) { + ret = -EINVAL; + goto out; + } - hred = *red++; - hgreen = *green++; - hblue = *blue++; + r = crtc->gamma_store; + g = r + crtc->gamma_size; + b = g + crtc->gamma_size; - if (transp) - htransp = *transp++; + memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(u16)); + memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(u16)); + memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(u16)); - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info); - if (rc) - goto out; - } - if (crtc_funcs->load_lut) - crtc_funcs->load_lut(crtc); + ret = crtc->funcs->gamma_set(crtc, r, g, b, + crtc->gamma_size, &ctx); + if (ret) + break; } - out: - drm_modeset_unlock_all(dev); - return rc; +out: + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return ret; } EXPORT_SYMBOL(drm_fb_helper_setcmap);
This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get totally obsolete. I think the gamma_store can end up invalid on error. But the way I read it, that can happen in drm_mode_gamma_set_ioctl as well, so why should this pesky legacy fbdev stuff be any better? drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However, it saves it to the gamma_store which should already be up to date with what .gamma_get would return and is thus a nop. So, zap it. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/drm_fb_helper.c | 131 ++++++++++++---------------------------- 1 file changed, 40 insertions(+), 91 deletions(-)