[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; }
return 0;
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-palettePeter 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-palettePeter 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
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-palettePeter 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:
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
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-palettePeter 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-palettePeter 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
linux-stable-mirror@lists.linaro.org
-
Daniel Vetter -
Sean Paul