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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
> >> > On Fri, Jan 12, 2018 at 4:48 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> >> >> [Fair warning: This is pure conjecture right now.]
> >> >>
> >> >> In
> >> >>
> >> >> commit b8e2b0199cc377617dc238f5106352c06dcd3fa2
> >> >> Author: Peter Rosin <peda@xxxxxxxxxx>
> >> >> 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@xxxxxxxxxx>
> >> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >> >> Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx>
> >> >> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> >> >> Cc: David Airlie <airlied@xxxxxxxx>
> >> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >> >> Cc: <stable@xxxxxxxxxxxxxxx> # v4.14+
> >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198123
> >> >> Reported-by: Deposite Pirate <dpirate@xxxxxxxxxxxxxxx>
> >> >> Reported-by: Bill Fraser <bill.fraser@xxxxxxxxx>
> >> >> Cc: Deposite Pirate <dpirate@xxxxxxxxxxxxxxx>
> >> >> Cc: Bill Fraser <bill.fraser@xxxxxxxxx>
> >> >> Cc: Michel Dänzer <michel@xxxxxxxxxxx>
> >> >> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >> >> ---
> >> >> 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
--
Sean Paul, Software Engineer, Google / Chromium OS