| Message ID | 1611838435-151774-1-git-send-email-zhangxuezhi3@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v12] staging: fbtft: add tearing signal detect | expand |
On Fri, 29 Jan 2021 16:26:12 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote: > > On Fri, 29 Jan 2021 12:23:08 +0200 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > We are almost there, I have no idea what Noralf or others are going to > say though. > > ... > > > Hi, I apologize for what I said in the previous two emails. I missed > > one email you sent before, and now I probably understand what you > > meant. Here is a version I modified according to your suggestion: > > > > From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00 > > 2001 From: Carlis <zhangxuezhi1@yulong.com> > > Date: Sun, 24 Jan 2021 22:43:21 +0800 > > Subject: [PATCH v13] staging: fbtft: add tearing signal detect > > > > For st7789v IC,when we need continuous full screen refresh, it is > > best > > Missed space after comma. > > > to wait for the tearing effect line signal arrive to avoid screen > > to arrive > > > tearing. > > ... > > > +#define PANEL_TE_TIMEOUT_MS 34 /* 60Hz for 16.6ms, configured as > > 2*16.6ms */ + > > Move comment before the definition > /* comment */ > #define DEFINITION > > Also consider to use 33 ms as closest to what you mentioned in the > comment. Or leave it with mention that you are using ceil() value. > > ... > > > +/** > > + * init_tearing_effect_line() - init tearing effect line > > > + * > > As per a few previous reviews. > Okay, I have noticed that the existing kernel-doc is written like > this, but it doesn't prevent you from avoiding this little mistake. > > > + * @par: FBTFT parameter object > > + * > > + * Return: 0 on success, or a negative error code otherwise. > > + */ > > +static int init_tearing_effect_line(struct fbtft_par *par) > > +{ > > + struct device *dev = par->info->device; > > + struct gpio_desc *te; > > + int rc; > > + > > + te = gpiod_get_optional(dev, "te", GPIOD_IN); > > + if (IS_ERR(te)) > > + return dev_err_probe(dev, PTR_ERR(te), "Failed to > > request te GPIO\n"); + > > Below is okay, but needs a comment explaining why we return a success. > > > + if (!te) > > + return 0; > > + > > + par->irq_te = gpiod_to_irq(te); > > + > > + /* GPIO is locked as an IRQ, we may drop the reference */ > > + gpiod_put(te); > > + > > + if (par->irq_te < 0) > > + return par->irq_te; > > I recommend using a temporary variable. In such a case you won't need > to specifically check for negative error code. So, something like > > int irq; > > irq = ... > > if (irq < 0) > return irq; > > ->irq_te = irq; > > > + init_completion(&par->panel_te); > > + rc = devm_request_irq(dev, par->irq_te, panel_te_handler, > > + IRQF_TRIGGER_RISING, "TE_GPIO", par); > > > > Right. Now it needs a comment explaining the choice of rising edge > type of IRQ. > > > + if (rc) > > + return dev_err_probe(dev, rc, "TE IRQ request > > failed.\n"); + > > + disable_irq_nosync(par->irq_te); > > + > > + return 0; > > +} > > ... > > > + rc = init_tearing_effect_line(par); > > > + if (rc < 0) > > Here is no need to specifically check against less than 0, > if (ret) > will work nicely. > > > + return rc; > > ... > > > + if (par->irq_te) > > + write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00); > > Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case? > Hi, TE line output is off by default when powering on, and I use Qualcomm SDX55 chip, SPI top speed is only 50MHz, its data transmission speed( It takes about 24ms to transmit a frame) is slower than 60Hz refresh(a frame only need 16.6ms), so i think there is no need to call MIPI_DCS_SET_TEAR_SCANLINE > Alos, when there is no IRQ, shouldn't we explicitly call > write_reg(par, MIPI_DCS_SET_TEAR_OFF); > ? > > ... > > > /** > > + * st7789v_write_vmem16_bus8() - write data to display > > > + * > > Redundant. > > > + * @par: FBTFT parameter object > > + * @offset: offset from screen_buffer > > + * @len: the length of data to be written > > + * > > > + * 16 bit pixel over 8-bit databus > > Write 16-bit pixels over 8-bit data bus. > > > + * Return: 0 on success, or a negative error code otherwise. > > + */ > > ... > > > + if (par->irq_te) { > > + enable_irq(par->irq_te); > > + reinit_completion(&par->panel_te); > > + ret = wait_for_completion_timeout(&par->panel_te, > > + > > msecs_to_jiffies(PANEL_TE_TIMEOUT_MS)); > > + if (ret == 0) > > + dev_err(dev, "wait panel TE time out\n"); > > timeout > > > + > > + disable_irq(par->irq_te); > > + } > > ... > > > + * @panel_te: completion for panel te line > > TE line > > > + * @irq_te: LCD Chip tearing effect line > > "Linux IRQ for LCD..." > hi, i modify new below: From b65fc2bfbe123defc8ec838601a9c12707f70c53 Mon Sep 17 00:00:00 2001 From: Carlis <zhangxuezhi1@yulong.com> Date: Sun, 24 Jan 2021 22:43:21 +0800 Subject: [PATCH v13] staging: fbtft: add tearing signal detect For st7789v IC, when we need continuous full screen refresh, it is best to wait for the tearing effect line signal to arrive to avoid screen tearing. Signed-off-by: Carlis <zhangxuezhi1@yulong.com> --- v13: change TE completion to par data struct member and add a new function to deal te gpio request, move wait logic to fbtft_write_vmem16_bus8. v12: change dev_err to dev_err_probe and add space in comments start, and delete te_mutex, change te wait logic. v11: remove devm_gpio_put and change a dev_err to dev_info. v10: additional notes. v9: change pr_* to dev_*. v8: delete a log line. v7: return error value when request fail. v6: add te gpio request fail deal logic. v5: fix log print. v4: modify some code style and change te irq set function name. v3: modify author and signed-off-by name. v2: add release te gpio after irq request fail. --- drivers/staging/fbtft/fb_st7789v.c | 64 ++++++++++++++++++++++++++++++++++++++ drivers/staging/fbtft/fbtft-bus.c | 20 ++++++++++-- drivers/staging/fbtft/fbtft.h | 5 +++ 3 files changed, 87 insertions(+), 2 deletions(-) diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..695dcac 100644 --- a/drivers/staging/fbtft/fb_st7789v.c +++ b/drivers/staging/fbtft/fb_st7789v.c @@ -7,9 +7,13 @@ #include <linux/bitops.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/interrupt.h> +#include <linux/completion.h> #include <linux/module.h> + #include <video/mipi_display.h> #include "fbtft.h" @@ -66,6 +70,56 @@ enum st7789v_command { #define MADCTL_MX BIT(6) /* bitmask for column address order */ #define MADCTL_MY BIT(7) /* bitmask for page address order */ +static irqreturn_t panel_te_handler(int irq, void *data) +{ + struct fbtft_par *par = (struct fbtft_par *)data; + + complete(&par->panel_te); + return IRQ_HANDLED; +} + +/* + * init_tearing_effect_line() - init tearing effect line. + * @par: FBTFT parameter object. + * + * Return: 0 on success, or a negative error code otherwise. + */ +static int init_tearing_effect_line(struct fbtft_par *par) +{ + struct device *dev = par->info->device; + struct gpio_desc *te; + int rc, irq; + + te = gpiod_get_optional(dev, "te", GPIOD_IN); + if (IS_ERR(te)) + return dev_err_probe(dev, PTR_ERR(te), "Failed to request te GPIO\n"); + + /* if te is NULL, indicating no configuration, directly return success */ + if (!te) + return 0; + + irq = gpiod_to_irq(te); + + /* GPIO is locked as an IRQ, we may drop the reference */ + gpiod_put(te); + + if (irq < 0) + return irq; + + par->irq_te = irq; + init_completion(&par->panel_te); + + /* The effective state is high and lasts no more than 1000 microseconds */ + rc = devm_request_irq(dev, par->irq_te, panel_te_handler, + IRQF_TRIGGER_RISING, "TE_GPIO", par); + if (rc) + return dev_err_probe(dev, rc, "TE IRQ request failed.\n"); + + disable_irq_nosync(par->irq_te); + + return 0; +} + /** * init_display() - initialize the display controller * @@ -82,6 +136,12 @@ enum st7789v_command { */ static int init_display(struct fbtft_par *par) { + int rc; + + rc = init_tearing_effect_line(par); + if (rc) + return rc; + /* turn off sleep mode */ write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE); mdelay(120); @@ -137,6 +197,10 @@ static int init_display(struct fbtft_par *par) */ write_reg(par, PWCTRL1, 0xA4, 0xA1); + /* TE line output is off by default when powering on */ + if (par->irq_te) + write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00); + write_reg(par, MIPI_DCS_SET_DISPLAY_ON); if (HSD20_IPS) diff --git a/drivers/staging/fbtft/fbtft-bus.c b/drivers/staging/fbtft/fbtft-bus.c index 63c65dd..5bc2fff 100644 --- a/drivers/staging/fbtft/fbtft-bus.c +++ b/drivers/staging/fbtft/fbtft-bus.c @@ -2,9 +2,15 @@ #include <linux/export.h> #include <linux/errno.h> #include <linux/gpio/consumer.h> +#include <linux/interrupt.h> +#include <linux/completion.h> #include <linux/spi/spi.h> + #include "fbtft.h" +/* 60Hz for 16.6ms, configured as 2*16.6ms */ +#define PANEL_TE_TIMEOUT_MS 33 + /***************************************************************************** * * void (*write_reg)(struct fbtft_par *par, int len, ...); @@ -135,8 +141,18 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len) remain = len / 2; vmem16 = (u16 *)(par->info->screen_buffer + offset); - if (par->gpio.dc) - gpiod_set_value(par->gpio.dc, 1); + gpiod_set_value(par->gpio.dc, 1); + + if (par->irq_te) { + enable_irq(par->irq_te); + reinit_completion(&par->panel_te); + ret = wait_for_completion_timeout(&par->panel_te, + msecs_to_jiffies(PANEL_TE_TIMEOUT_MS)); + if (ret == 0) + dev_err(par->info->device, "wait panel TE timeout\n"); + + disable_irq(par->irq_te); + } /* non buffered write */ if (!par->txbuf.buf) diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h index 76f8c09..61c7207 100644 --- a/drivers/staging/fbtft/fbtft.h +++ b/drivers/staging/fbtft/fbtft.h @@ -8,6 +8,7 @@ #include <linux/spinlock.h> #include <linux/spi/spi.h> #include <linux/platform_device.h> +#include <linux/completion.h> #define FBTFT_ONBOARD_BACKLIGHT 2 @@ -165,6 +166,8 @@ struct fbtft_platform_data { * Format: 6 bit Device id + RS bit + RW bit * @fbtftops: FBTFT operations provided by driver or device (platform_data) * @dirty_lock: Protects dirty_lines_start and dirty_lines_end + * @panel_te: completion for panel TE line + * @irq_te: Linux IRQ for LCD TE line * @dirty_lines_start: Where to begin updating display * @dirty_lines_end: Where to end updating display * @gpio.reset: GPIO used to reset display @@ -203,6 +206,8 @@ struct fbtft_par { u8 startbyte; struct fbtft_ops fbtftops; spinlock_t dirty_lock; + struct completion panel_te; + int irq_te; unsigned int dirty_lines_start; unsigned int dirty_lines_end; struct { -- 1.9.1 regards, Carlis
On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote: > On Fri, 29 Jan 2021 16:26:12 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> wrote: > > > On Fri, 29 Jan 2021 12:23:08 +0200 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > > > Hi, I apologize for what I said in the previous two emails. I missed > > > one email you sent before, and now I probably understand what you > > > meant. Here is a version I modified according to your suggestion: I have realized that you are mocking stuff in the generic fbtft structure for all drivers while only a single one is going to use that. Consider moving everything to the driver in question.
On Mon, 1 Feb 2021 19:40:21 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote: > > On Fri, 29 Jan 2021 16:26:12 +0200 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> > > > wrote: > > > > On Fri, 29 Jan 2021 12:23:08 +0200 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > ... > > > > > Hi, I apologize for what I said in the previous two emails. I > > > > missed one email you sent before, and now I probably understand > > > > what you meant. Here is a version I modified according to your > > > > suggestion: > > I have realized that you are mocking stuff in the generic fbtft > structure for all drivers while only a single one is going to use > that. Consider moving everything to the driver in question. > > hi, Do you mean that i define the TE completion and irq_te in the fb_st7789v.c as i did before?
On Tue, Feb 2, 2021 at 3:52 AM Carlis <zhangxuezhi3@gmail.com> wrote: > On Mon, 1 Feb 2021 19:40:21 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Sat, Jan 30, 2021 at 8:39 AM carlis <zhangxuezhi3@gmail.com> wrote: > > > On Fri, 29 Jan 2021 16:26:12 +0200 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Jan 29, 2021 at 3:56 PM carlis <zhangxuezhi3@gmail.com> > > > > wrote: > > > > > On Fri, 29 Jan 2021 12:23:08 +0200 > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > ... > > > > > > > Hi, I apologize for what I said in the previous two emails. I > > > > > missed one email you sent before, and now I probably understand > > > > > what you meant. Here is a version I modified according to your > > > > > suggestion: > > > > I have realized that you are mocking stuff in the generic fbtft > > structure for all drivers while only a single one is going to use > > that. Consider moving everything to the driver in question. > Do you mean that i define the TE completion and irq_te in the > fb_st7789v.c as i did before? Not in global variables. Perhaps it will require to add/update the custom (to the specific driver) data structure. But the idea is that all changes should be isolated to that driver.
diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c index 3a280cc..f08e9da 100644 --- a/drivers/staging/fbtft/fb_st7789v.c +++ b/drivers/staging/fbtft/fb_st7789v.c @@ -9,7 +9,11 @@ #include <linux/delay.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/interrupt.h> +#include <linux/completion.h> #include <linux/module.h> +#include <linux/gpio/consumer.h> + #include <video/mipi_display.h> #include "fbtft.h" @@ -66,6 +70,15 @@ enum st7789v_command { #define MADCTL_MX BIT(6) /* bitmask for column address order */ #define MADCTL_MY BIT(7) /* bitmask for page address order */ +#define SPI_PANEL_TE_TIMEOUT 400 /* msecs */ +static struct completion spi_panel_te; + +static irqreturn_t spi_panel_te_handler(int irq, void *data) +{ + complete(&spi_panel_te); + return IRQ_HANDLED; +} + /** * init_display() - initialize the display controller * @@ -82,6 +95,29 @@ enum st7789v_command { */ static int init_display(struct fbtft_par *par) { + int rc; + struct device *dev = par->info->device; + + par->gpio.te = devm_gpiod_get_index_optional(dev, "te", 0, GPIOD_IN); + if (IS_ERR(par->gpio.te)) + return dev_err_probe(par->info->device, PTR_ERR(par->gpio.te), + "Failed to request te gpio\n"); + + if (par->gpio.te) { + init_completion(&spi_panel_te); + rc = devm_request_irq(dev, + gpiod_to_irq(par->gpio.te), + spi_panel_te_handler, IRQF_TRIGGER_RISING, + "TE_GPIO", par); + if (IS_ERR(rc)) + return dev_err_probe(par->info->device, PTR_ERR(rc), + "TE request_irq failed.\n"); + + disable_irq_nosync(gpiod_to_irq(par->gpio.te)); + } else { + dev_info(par->info->device, "%s:%d, TE gpio not specified\n", + __func__, __LINE__); + } /* turn off sleep mode */ write_reg(par, MIPI_DCS_EXIT_SLEEP_MODE); mdelay(120); @@ -137,6 +173,10 @@ static int init_display(struct fbtft_par *par) */ write_reg(par, PWCTRL1, 0xA4, 0xA1); + /* tearing effect line on */ + if (par->gpio.te) + write_reg(par, 0x35, 0x00); + write_reg(par, MIPI_DCS_SET_DISPLAY_ON); if (HSD20_IPS) @@ -146,6 +186,81 @@ static int init_display(struct fbtft_par *par) } /** + * st7789v_write_vmem16_bus8() - write data to display + * + * @par: FBTFT parameter object + * @offset: offset from screen_buffer + * @len: the length of data to be written + * + * 16 bit pixel over 8-bit databus + * + * Return: 0 on success, < 0 if error occurred. + */ + +static int st7789v_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len) +{ + u16 *vmem16; + __be16 *txbuf16 = par->txbuf.buf; + size_t remain; + size_t to_copy; + size_t tx_array_size; + int i; + int ret = 0; + size_t startbyte_size = 0; + + fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, len=%zu)\n", + __func__, offset, len); + + remain = len / 2; + vmem16 = (u16 *)(par->info->screen_buffer + offset); + + if (par->gpio.dc) + gpiod_set_value(par->gpio.dc, 1); + + if (par->gpio.te) { + enable_irq(gpiod_to_irq(par->gpio.te)); + reinit_completion(&spi_panel_te); + ret = wait_for_completion_timeout(&spi_panel_te, + msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT)); + if (ret == 0) + dev_err(par->info->device, "wait panel TE time out\n"); + + disable_irq(gpiod_to_irq(par->gpio.te)); + } + /* non buffered write */ + if (!par->txbuf.buf) + return par->fbtftops.write(par, vmem16, len); + + /* buffered write */ + tx_array_size = par->txbuf.len / 2; + + if (par->startbyte) { + txbuf16 = par->txbuf.buf + 1; + tx_array_size -= 2; + *(u8 *)(par->txbuf.buf) = par->startbyte | 0x2; + startbyte_size = 1; + } + + while (remain) { + to_copy = min(tx_array_size, remain); + dev_dbg(par->info->device, " to_copy=%zu, remain=%zu\n", + to_copy, remain - to_copy); + + for (i = 0; i < to_copy; i++) + txbuf16[i] = cpu_to_be16(vmem16[i]); + + vmem16 = vmem16 + to_copy; + ret = par->fbtftops.write(par, par->txbuf.buf, + startbyte_size + to_copy * 2); + if (ret < 0) + return ret; + remain -= to_copy; + } + + return ret; +} + +/** * set_var() - apply LCD properties like rotation and BGR mode * * @par: FBTFT parameter object @@ -259,6 +374,7 @@ static int blank(struct fbtft_par *par, bool on) .gamma = HSD20_IPS_GAMMA, .fbtftops = { .init_display = init_display, + .write_vmem = st7789v_write_vmem16_bus8, .set_var = set_var, .set_gamma = set_gamma, .blank = blank, diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h index 76f8c09..93bac05 100644 --- a/drivers/staging/fbtft/fbtft.h +++ b/drivers/staging/fbtft/fbtft.h @@ -212,6 +212,7 @@ struct fbtft_par { struct gpio_desc *wr; struct gpio_desc *latch; struct gpio_desc *cs; + struct gpio_desc *te; struct gpio_desc *db[16]; struct gpio_desc *led[16]; struct gpio_desc *aux[16];