| Message ID | 20180112094834.26310-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 12, 2018 at 4:48 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > [Fair warning: This is pure conjecture right now.] > > In > > commit b8e2b0199cc377617dc238f5106352c06dcd3fa2 > Author: Peter Rosin <peda@axentia.se> > Date: Tue Jul 4 12:36:57 2017 +0200 > > drm/fb-helper: factor out pseudo-palette > > Peter extracted the pseudo palette computation, but seems to have done > an off-by-one. I spotted that +1, but then noticed that we've passed > start++ to (now gone) setcolreg function, so it seemed to all match > up. Except post vs. pre-increment ftw. > > Result is that the palette is off-by-one, and the forground color > (slot 0) ends up black, rendering a fairly unreadable console. > > What baffles me is that on some systems it still seems to work > somehow, which lead us all down a wild goose chase trying to add > load_lut calls back in in various places (which was also intentionally > removed, but really doesn't seem to be the real root cause). > > Fixes: b8e2b0199cc3 ("drm/fb-helper: factor out pseudo-palette") > Cc: Peter Rosin <peda@axenita.se> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Cc: <stable@vger.kernel.org> # v4.14+ > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198123 > Reported-by: Deposite Pirate <dpirate@metalpunks.info> > Reported-by: Bill Fraser <bill.fraser@gmail.com> > Cc: Deposite Pirate <dpirate@metalpunks.info> > Cc: Bill Fraser <bill.fraser@gmail.com> > Cc: Michel Dänzer <michel@daenzer.net> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 035784ddd133..1c3a200c4a10 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1295,7 +1295,7 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) > mask <<= info->var.transp.offset; > value |= mask; > } > - palette[cmap->start + i] = value; > + palette[cmap->start] = value; I don't think this is equivalent to what was there before. Before there we set palette[cmap->start]->palette[cmap->start + cmap->len - 1], now we're only setting palette[cmap->start]. In the previous version (before Peter's change), we wrote palette[cmap->start] twice. So while there is an off-by-one bug, I don't think this fixes the issue. Sean > } > > return 0; > -- > 2.15.1 >
On Fri, Jan 12, 2018 at 3:08 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Fri, Jan 12, 2018 at 4:48 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> [Fair warning: This is pure conjecture right now.] >> >> In >> >> commit b8e2b0199cc377617dc238f5106352c06dcd3fa2 >> Author: Peter Rosin <peda@axentia.se> >> Date: Tue Jul 4 12:36:57 2017 +0200 >> >> drm/fb-helper: factor out pseudo-palette >> >> Peter extracted the pseudo palette computation, but seems to have done >> an off-by-one. I spotted that +1, but then noticed that we've passed >> start++ to (now gone) setcolreg function, so it seemed to all match >> up. Except post vs. pre-increment ftw. >> >> Result is that the palette is off-by-one, and the forground color >> (slot 0) ends up black, rendering a fairly unreadable console. >> >> What baffles me is that on some systems it still seems to work >> somehow, which lead us all down a wild goose chase trying to add >> load_lut calls back in in various places (which was also intentionally >> removed, but really doesn't seem to be the real root cause). >> >> Fixes: b8e2b0199cc3 ("drm/fb-helper: factor out pseudo-palette") >> Cc: Peter Rosin <peda@axenita.se> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Gustavo Padovan <gustavo@padovan.org> >> Cc: Sean Paul <seanpaul@chromium.org> >> Cc: David Airlie <airlied@linux.ie> >> Cc: dri-devel@lists.freedesktop.org >> Cc: <stable@vger.kernel.org> # v4.14+ >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198123 >> Reported-by: Deposite Pirate <dpirate@metalpunks.info> >> Reported-by: Bill Fraser <bill.fraser@gmail.com> >> Cc: Deposite Pirate <dpirate@metalpunks.info> >> Cc: Bill Fraser <bill.fraser@gmail.com> >> Cc: Michel Dänzer <michel@daenzer.net> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 035784ddd133..1c3a200c4a10 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1295,7 +1295,7 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) >> mask <<= info->var.transp.offset; >> value |= mask; >> } >> - palette[cmap->start + i] = value; >> + palette[cmap->start] = value; > > I don't think this is equivalent to what was there before. Before > there we set palette[cmap->start]->palette[cmap->start + cmap->len - > 1], now we're only setting palette[cmap->start]. > > In the previous version (before Peter's change), we wrote > palette[cmap->start] twice. So while there is an off-by-one bug, I > don't think this fixes the issue. Well this patch is complete nonsense, no idea what I was thinking ... -Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 035784ddd133..1c3a200c4a10 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1295,7 +1295,7 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) mask <<= info->var.transp.offset; value |= mask; } - palette[cmap->start + i] = value; + palette[cmap->start] = value; } return 0;
[Fair warning: This is pure conjecture right now.] In commit b8e2b0199cc377617dc238f5106352c06dcd3fa2 Author: Peter Rosin <peda@axentia.se> Date: Tue Jul 4 12:36:57 2017 +0200 drm/fb-helper: factor out pseudo-palette Peter extracted the pseudo palette computation, but seems to have done an off-by-one. I spotted that +1, but then noticed that we've passed start++ to (now gone) setcolreg function, so it seemed to all match up. Except post vs. pre-increment ftw. Result is that the palette is off-by-one, and the forground color (slot 0) ends up black, rendering a fairly unreadable console. What baffles me is that on some systems it still seems to work somehow, which lead us all down a wild goose chase trying to add load_lut calls back in in various places (which was also intentionally removed, but really doesn't seem to be the real root cause). Fixes: b8e2b0199cc3 ("drm/fb-helper: factor out pseudo-palette") Cc: Peter Rosin <peda@axenita.se> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Gustavo Padovan <gustavo@padovan.org> Cc: Sean Paul <seanpaul@chromium.org> Cc: David Airlie <airlied@linux.ie> Cc: dri-devel@lists.freedesktop.org Cc: <stable@vger.kernel.org> # v4.14+ Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198123 Reported-by: Deposite Pirate <dpirate@metalpunks.info> Reported-by: Bill Fraser <bill.fraser@gmail.com> Cc: Deposite Pirate <dpirate@metalpunks.info> Cc: Bill Fraser <bill.fraser@gmail.com> Cc: Michel Dänzer <michel@daenzer.net> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_fb_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)