| Message ID | 20190723133755.22677-2-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RFT: PL111 DRM conversion of nspire | expand |
Hi,
I gave the series a try (virtual CX and TP so far, will do on a real CX later):
OOPS with a nullptr deref during probe.
This diff which just moves some lines around fixes that and the LCD appears to
work properly.
Once I verified the 24bit depth and clock speed config on HW as well, I can
give you my Tested-by, or would you prefer that I resubmit your series with the
fix below?
Thanks,
Fabian
---
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index 587b4d148c18..bd84d7a5a0f4 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -133,10 +133,6 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
u32 cpl, tim2;
int ret;
- if (connector->display_info.num_bus_formats == 1 &&
- connector->display_info.bus_formats[0] == MEDIA_BUS_FMT_Y8_1X8)
- grayscale = true;
-
ret = clk_set_rate(priv->clk, mode->clock * 1000);
if (ret) {
dev_err(drm->dev,
@@ -191,6 +187,10 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
tim2 |= TIM2_IPC;
+ if (connector->display_info.num_bus_formats == 1 &&
+ connector->display_info.bus_formats[0] == MEDIA_BUS_FMT_Y8_1X8)
+ grayscale = true;
+
/*
* The AC pin bias frequency is set to max count when using
* grayscale so at least once in a while we will reverse
On 7/23/19 8:37 AM, Linus Walleij wrote: > Migrating the TI nspire calculators to use the PL111 driver for > framebuffer requires grayscale support for the elder panel > which uses 8bit grayscale only. > > DRM does not support 8bit grayscale framebuffers in memory, > but by defining the bus format to be MEDIA_BUS_FMT_Y8_1X8 we > can get the hardware to turn on a grayscaling feature and > convert the RGB framebuffer to grayscale for us. > What would it take to add proper grayscale framebuffer support to DRM? I've been using the RGB to gray conversion method for a while now with st7586 and it is OK but is creates extra work if you want things to actually look "good" instead of "OK" because you have to add code to userspace programs to craft images in a certain way so that they come out on the other side looking as intended on the actual display.
On Tue, 2019-07-23 at 15:37 +0200, Linus Walleij wrote: > Migrating the TI nspire calculators to use the PL111 driver for > framebuffer requires grayscale support for the elder panel > which uses 8bit grayscale only. > > DRM does not support 8bit grayscale framebuffers in memory, > but by defining the bus format to be MEDIA_BUS_FMT_Y8_1X8 we > can get the hardware to turn on a grayscaling feature and > convert the RGB framebuffer to grayscale for us. What's wrong with DRM_FORMAT_R8? Yes the hardware is not really "redscale", but it's still a single color channel and there's not really any ambiguity. - ajax
On Tue, Jul 23, 2019 at 7:25 PM Adam Jackson <ajax@redhat.com> wrote: > > On Tue, 2019-07-23 at 15:37 +0200, Linus Walleij wrote: > > Migrating the TI nspire calculators to use the PL111 driver for > > framebuffer requires grayscale support for the elder panel > > which uses 8bit grayscale only. > > > > DRM does not support 8bit grayscale framebuffers in memory, > > but by defining the bus format to be MEDIA_BUS_FMT_Y8_1X8 we > > can get the hardware to turn on a grayscaling feature and > > convert the RGB framebuffer to grayscale for us. > > What's wrong with DRM_FORMAT_R8? Yes the hardware is not really > "redscale", but it's still a single color channel and there's not > really any ambiguity. Yeah, I think with a comment or an aliasing #define to _Y8 (or both) this is good to go. You probably still want to expose the rgb format since too much userspace just assumes that xrgb8888 works. Same reason why the tinydrm drivers do the sw conversion. -Daniel
On Tue, Jul 23, 2019 at 5:19 PM Fabian Vogt <fabian@ritter-vogt.de> wrote: > I gave the series a try (virtual CX and TP so far, will do on a real CX later): > OOPS with a nullptr deref during probe. > This diff which just moves some lines around fixes that and the LCD appears to > work properly. OK I folded this into my patch, thanks! > Once I verified the 24bit depth and clock speed config on HW as well, I can > give you my Tested-by, or would you prefer that I resubmit your series with the > fix below? It's fine if you test it just with your patch as-is, I didn't change anything else. I would be amazed if it "just works" now. Yours, Linus Walleij
On Tue, Jul 23, 2019 at 11:07 PM Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jul 23, 2019 at 7:25 PM Adam Jackson <ajax@redhat.com> wrote: > > > > On Tue, 2019-07-23 at 15:37 +0200, Linus Walleij wrote: > > > Migrating the TI nspire calculators to use the PL111 driver for > > > framebuffer requires grayscale support for the elder panel > > > which uses 8bit grayscale only. > > > > > > DRM does not support 8bit grayscale framebuffers in memory, > > > but by defining the bus format to be MEDIA_BUS_FMT_Y8_1X8 we > > > can get the hardware to turn on a grayscaling feature and > > > convert the RGB framebuffer to grayscale for us. > > > > What's wrong with DRM_FORMAT_R8? Yes the hardware is not really > > "redscale", but it's still a single color channel and there's not > > really any ambiguity. > > Yeah, I think with a comment or an aliasing #define to _Y8 (or both) > this is good to go. Is there something really wrong with just biting the bullet and do this: /* 8 bpp grayscale */ #define DRM_FORMAT_Y8 fourcc_code('Y', '8', ' ', ' ') /* [7:0] Y */ It's quite an embarrasement for my OCD tendencies to talk about black-and-white TV as if it was 256 Shades of Red (good title for a novel by the way). I don't know how these FOURCC things work, possibly new fourcc:s can only be defined by some especially enlightened cabal of standardization? (It beats me how it can not already exist in that case.) > You probably still want to expose the rgb format since too much > userspace just assumes that xrgb8888 works. Same reason why the > tinydrm drivers do the sw conversion. Yes this is what we do on PL111 now, we just run it through the hardware grayscaler. This hardware graciously supports reading black-white and grayscale bitmaps with 1 (monochrome), 2, 4 and 8 bits per pixel which would be Y1, Y2, Y4 and Y8. But we only have hardware supporting Y8 at least on the other side so I don't see any need for the others ATM. I suspect the Y1 etc could be useful for people doing not only Hercules video drivers (hah!) but also for ePaper displays of say, some random Kindle. Yours, Linus Walleij
On Wed, Jul 24, 2019 at 2:52 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jul 23, 2019 at 11:07 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Jul 23, 2019 at 7:25 PM Adam Jackson <ajax@redhat.com> wrote: > > > > > > On Tue, 2019-07-23 at 15:37 +0200, Linus Walleij wrote: > > > > Migrating the TI nspire calculators to use the PL111 driver for > > > > framebuffer requires grayscale support for the elder panel > > > > which uses 8bit grayscale only. > > > > > > > > DRM does not support 8bit grayscale framebuffers in memory, > > > > but by defining the bus format to be MEDIA_BUS_FMT_Y8_1X8 we > > > > can get the hardware to turn on a grayscaling feature and > > > > convert the RGB framebuffer to grayscale for us. > > > > > > What's wrong with DRM_FORMAT_R8? Yes the hardware is not really > > > "redscale", but it's still a single color channel and there's not > > > really any ambiguity. > > > > Yeah, I think with a comment or an aliasing #define to _Y8 (or both) > > this is good to go. > > Is there something really wrong with just biting the bullet and do this: > > /* 8 bpp grayscale */ > #define DRM_FORMAT_Y8 fourcc_code('Y', '8', ' ', ' ') /* [7:0] Y */ > > It's quite an embarrasement for my OCD tendencies to talk about > black-and-white TV as if it was 256 Shades of Red (good title > for a novel by the way). > > I don't know how these FOURCC things work, possibly new > fourcc:s can only be defined by some especially enlightened > cabal of standardization? > > (It beats me how it can not already exist in that case.) The drm subsystem cabal owns drm_fourcc.h. And yeah I guess we can also add Y version of all these, I think the R/RG was added since that matches modern GL, where your texture contents are entirely up to interpretation by shaders. Y also exists in GL, but only in legacy contexts and is kinda discouraged. That was the idea behind just making them aliasing (since I just can't come up with any reason why you'd actually want a red-only texture). In a way R = red = the first channel in GL shaders, which happens to be called r for red != actually red color channel on the display. Hence we might as well state that if you give a kms driver a single-channel fourcc code, then that should be interpreted as greyscale. Plus add a huge comment about why this single channel is called R :-) -Daniel > > You probably still want to expose the rgb format since too much > > userspace just assumes that xrgb8888 works. Same reason why the > > tinydrm drivers do the sw conversion. > > Yes this is what we do on PL111 now, we just run it through > the hardware grayscaler. > > This hardware graciously supports reading black-white and > grayscale bitmaps with 1 (monochrome), 2, 4 and 8 bits per > pixel which would be Y1, Y2, Y4 and Y8. But we only have > hardware supporting Y8 at least on the other side so > I don't see any need for the others ATM. > > I suspect the Y1 etc could be useful for people doing not > only Hercules video drivers (hah!) but also for ePaper > displays of say, some random Kindle. I guess if we bother with Y (whether aliased to R or new ones) we might as well roll out all of them. -Daniel
Hi, Am Mittwoch, 24. Juli 2019, 14:33:06 CEST schrieb Linus Walleij: > On Tue, Jul 23, 2019 at 5:19 PM Fabian Vogt <fabian@ritter-vogt.de> wrote: > > > I gave the series a try (virtual CX and TP so far, will do on a real CX later): > > OOPS with a nullptr deref during probe. > > This diff which just moves some lines around fixes that and the LCD appears to > > work properly. > > OK I folded this into my patch, thanks! > > > Once I verified the 24bit depth and clock speed config on HW as well, I can > > give you my Tested-by, or would you prefer that I resubmit your series with the > > fix below? > > It's fine if you test it just with your patch as-is, I didn't change anything > else. > > I would be amazed if it "just works" now. Indeed, you won't be. On a real CX the LCD displays shows content without any other changes to the set, but has a ~3Hz pulsating effect scrolling from the top to the bottom and the gray text changes color slightly. Without the patchset applied everything looks perfectly. I tried setting vrefresh to 20, fb_bpp to 16 and forcing an inverted panel clock, but the pulsing didn't change. Using the emulated CX I compared the contents of the registers and found that only the IPC bit (which I tried to set, so that's likely not it) and the interrupt registers have a different content. Any idea? Thanks, Fabian > Yours, > Linus Walleij
On Thu, Jul 25, 2019 at 9:26 PM Fabian Vogt <fabian@ritter-vogt.de> wrote: > On a real CX the LCD displays shows content without > any other changes to the set, but has a ~3Hz pulsating effect scrolling from > the top to the bottom and the gray text changes color slightly. So you mean something meaningful appears in the LCD but there are visual disturbances? > Without the patchset applied everything looks perfectly. > > I tried setting vrefresh to 20, fb_bpp to 16 and forcing an inverted panel > clock, but the pulsing didn't change. > > Using the emulated CX I compared the contents of the registers and found > that only the IPC bit (which I tried to set, so that's likely not it) and > the interrupt registers have a different content. > > Any idea? I think it's the clock settings in patch 2/3. I just set them to "1000" (1MHz since its in kHz) based on some educated guesses. The old driver set the clock to "1" (kHz) so just try that first, maybe it is really that slow. It's just that the old driver also set refres to 60 fps which doesn't add up, but I think that setting is simply ignored and that is why it works. Yours, Linus Walleij Yours, Linus Walleij Yours, Linus Walleij
Hi, Am Samstag, 3. August 2019, 11:51:59 CEST schrieb Linus Walleij: > On Thu, Jul 25, 2019 at 9:26 PM Fabian Vogt <fabian@ritter-vogt.de> wrote: > > > On a real CX the LCD displays shows content without > > any other changes to the set, but has a ~3Hz pulsating effect scrolling from > > the top to the bottom and the gray text changes color slightly. > > So you mean something meaningful appears in the LCD > but there are visual disturbances? > > > Without the patchset applied everything looks perfectly. > > > > I tried setting vrefresh to 20, fb_bpp to 16 and forcing an inverted panel > > clock, but the pulsing didn't change. > > > > Using the emulated CX I compared the contents of the registers and found > > that only the IPC bit (which I tried to set, so that's likely not it) and > > the interrupt registers have a different content. > > > > Any idea? > > I think it's the clock settings in patch 2/3. > > I just set them to "1000" (1MHz since its in kHz) based on some > educated guesses. > > The old driver set the clock to "1" (kHz) so just try that first, > maybe it is really that slow. Did that, it looked rather unhealthy for the LCD. Mostly white with some glitching except for ~2-3 lines with content scrolling on the screen. 200kHz was still way to slow, so tried the opposite direction. With a clock of 10MHz the display seems to be working fine and produces a visibly stable output. Thanks, Fabian > It's just that the old driver also set refres to 60 fps which doesn't > add up, but I think that setting is simply ignored and that is why > it works. > > Yours, > Linus Walleij --- diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index e5cfe1398a3b..3addcdab8adb 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -2763,7 +2763,7 @@ static const struct panel_desc arm_rtsm = { static const struct drm_display_mode nspire_cx_lcd_mode[] = { { - .clock = 1000, + .clock = 10000, .hdisplay = 320, .hsync_start = 320 + 50, .hsync_end = 320 + 50 + 6, @@ -2791,7 +2791,7 @@ static const struct panel_desc nspire_cx_lcd_panel = { static const struct drm_display_mode nspire_classic_lcd_mode[] = { { - .clock = 1000, + .clock = 10000, .hdisplay = 320, .hsync_start = 320 + 6, .hsync_end = 320 + 6 + 6,
diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c index 15d2755fdba4..587b4d148c18 100644 --- a/drivers/gpu/drm/pl111/pl111_display.c +++ b/drivers/gpu/drm/pl111/pl111_display.c @@ -126,12 +126,17 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe, struct drm_framebuffer *fb = plane->state->fb; struct drm_connector *connector = priv->connector; struct drm_bridge *bridge = priv->bridge; + bool grayscale = false; u32 cntl; u32 ppl, hsw, hfp, hbp; u32 lpp, vsw, vfp, vbp; u32 cpl, tim2; int ret; + if (connector->display_info.num_bus_formats == 1 && + connector->display_info.bus_formats[0] == MEDIA_BUS_FMT_Y8_1X8) + grayscale = true; + ret = clk_set_rate(priv->clk, mode->clock * 1000); if (ret) { dev_err(drm->dev, @@ -185,6 +190,15 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe, if (connector->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) tim2 |= TIM2_IPC; + + /* + * The AC pin bias frequency is set to max count when using + * grayscale so at least once in a while we will reverse + * polarity and get rid of any DC built up that could + * damage the display. + */ + if (grayscale) + tim2 |= TIM2_ACB_MASK; } if (bridge) { @@ -216,8 +230,18 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe, writel(0, priv->regs + CLCD_TIM3); - /* Hard-code TFT panel */ - cntl = CNTL_LCDEN | CNTL_LCDTFT | CNTL_LCDVCOMP(1); + /* + * Detect grayscale bus format. We do not support a grayscale mode + * toward userspace, instead we expose an RGB24 buffer and then the + * hardware will activate its grayscaler to convert to the grayscale + * format. + */ + if (grayscale) + cntl = CNTL_LCDEN | CNTL_LCDMONO8; + else + /* Else we assume TFT display */ + cntl = CNTL_LCDEN | CNTL_LCDTFT | CNTL_LCDVCOMP(1); + /* On the ST Micro variant, assume all 24 bits are connected */ if (priv->variant->st_bitmux_control) cntl |= CNTL_ST_CDWID_24; diff --git a/include/linux/amba/clcd-regs.h b/include/linux/amba/clcd-regs.h index 516a6fda83c5..421b0fa90d6a 100644 --- a/include/linux/amba/clcd-regs.h +++ b/include/linux/amba/clcd-regs.h @@ -42,6 +42,7 @@ #define TIM2_PCD_LO_MASK GENMASK(4, 0) #define TIM2_PCD_LO_BITS 5 #define TIM2_CLKSEL (1 << 5) +#define TIM2_ACB_MASK GENMASK(10, 6) #define TIM2_IVS (1 << 11) #define TIM2_IHS (1 << 12) #define TIM2_IPC (1 << 13)
Migrating the TI nspire calculators to use the PL111 driver for framebuffer requires grayscale support for the elder panel which uses 8bit grayscale only. DRM does not support 8bit grayscale framebuffers in memory, but by defining the bus format to be MEDIA_BUS_FMT_Y8_1X8 we can get the hardware to turn on a grayscaling feature and convert the RGB framebuffer to grayscale for us. Cc: Daniel Tang <dt.tangr@gmail.com> Cc: Fabian Vogt <fabian@ritter-vogt.de> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpu/drm/pl111/pl111_display.c | 28 +++++++++++++++++++++++++-- include/linux/amba/clcd-regs.h | 1 + 2 files changed, 27 insertions(+), 2 deletions(-)