| Message ID | 20180116220032.ceu2lay3jfsjncj4@art_vandelay (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 16, 2018 at 11:00 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Fri, Jan 12, 2018 at 10:08:49PM +0100, Daniel Vetter wrote: >> 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 ... > > [Sending this out since it's in my tree and I'll forget soon] > > Equivalent behavior would be: Are you sure? On 2nd look I think there isn't actually any off-by-one going on in the referenced patch. The only thing I can see is that the old code filled out the first entries of an oversized table (more than 16 entries), and only returned -EINVAL when getting to entry 17. The new one doesn't do that. But more testing in the bug report hints at this not being the real bug (and I didn't find any callers in the fbcon code that would indicate there is such a bug). -Daniel > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 035784ddd133..df6709768723 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1295,7 +1295,8 @@ 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; > + if (i > 0) > + palette[cmap->start + i - 1] = value; > } > > return 0; > > >> -Daniel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > -- > Sean Paul, Software Engineer, Google / Chromium OS
On Tue, Jan 16, 2018 at 11:27:28PM +0100, Daniel Vetter wrote: > On Tue, Jan 16, 2018 at 11:00 PM, Sean Paul <seanpaul@chromium.org> wrote: > > On Fri, Jan 12, 2018 at 10:08:49PM +0100, Daniel Vetter wrote: > >> 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 ... > > > > [Sending this out since it's in my tree and I'll forget soon] > > > > Equivalent behavior would be: > > Are you sure? Definitely not, I was just driving by, enjoying the scenery. > On 2nd look I think there isn't actually any off-by-one Yeah, I have convinced myself of the same thing. So I'll just file this under ¯\_(ツ)_/¯. Sean > going on in the referenced patch. The only thing I can see is that the > old code filled out the first entries of an oversized table (more than > 16 entries), and only returned -EINVAL when getting to entry 17. The > new one doesn't do that. But more testing in the bug report hints at > this not being the real bug (and I didn't find any callers in the > fbcon code that would indicate there is such a bug). > -Daniel > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 035784ddd133..df6709768723 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -1295,7 +1295,8 @@ 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; > > + if (i > 0) > > + palette[cmap->start + i - 1] = value; > > } > > > > return 0; > > > > > >> -Daniel > >> -- > >> Daniel Vetter > >> Software Engineer, Intel Corporation > >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 035784ddd133..df6709768723 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1295,7 +1295,8 @@ 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; + if (i > 0) + palette[cmap->start + i - 1] = value; } return 0;