Patches 1-4 fixes a few things that came up during the tinydrm review but wasn't addressed at the time.
Noralf.
Noralf Trønnes (5): drm/tinydrm/mi0283qt: Use common include order drm/tinydrm/mi0283qt: Remove ili9341.h drm/tinydrm/mi0283qt: Clarify error message drm/tinydrm/mi0283qt: Let the display pipe handle power drm/tinydrm: Embed the mode in tinydrm_connector
drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 ++++------- drivers/gpu/drm/tinydrm/mi0283qt.c | 92 ++++++++++++++--------------- drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 +++-- include/drm/tinydrm/ili9341.h | 54 ----------------- 4 files changed, 65 insertions(+), 129 deletions(-) delete mode 100644 include/drm/tinydrm/ili9341.h
Include linux headers before drm headers as it's commonly done.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/mi0283qt.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 674d407640be..45f02b6052b1 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -9,17 +9,18 @@ * (at your option) any later version. */
-#include <drm/drm_fb_helper.h> -#include <drm/drm_modeset_helper.h> -#include <drm/tinydrm/ili9341.h> -#include <drm/tinydrm/mipi-dbi.h> -#include <drm/tinydrm/tinydrm-helpers.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> #include <linux/property.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> + +#include <drm/drm_fb_helper.h> +#include <drm/drm_modeset_helper.h> +#include <drm/tinydrm/ili9341.h> +#include <drm/tinydrm/mipi-dbi.h> +#include <drm/tinydrm/tinydrm-helpers.h> #include <video/mipi_display.h>
static int mi0283qt_init(struct mipi_dbi *mipi)
On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
Include linux headers before drm headers as it's commonly done.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/mi0283qt.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 674d407640be..45f02b6052b1 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -9,17 +9,18 @@
- (at your option) any later version.
*/
-#include <drm/drm_fb_helper.h> -#include <drm/drm_modeset_helper.h> -#include <drm/tinydrm/ili9341.h> -#include <drm/tinydrm/mipi-dbi.h> -#include <drm/tinydrm/tinydrm-helpers.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> #include <linux/property.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h>
+#include <drm/drm_fb_helper.h> +#include <drm/drm_modeset_helper.h> +#include <drm/tinydrm/ili9341.h> +#include <drm/tinydrm/mipi-dbi.h> +#include <drm/tinydrm/tinydrm-helpers.h> #include <video/mipi_display.h>
static int mi0283qt_init(struct mipi_dbi *mipi)
Reviewed-by: David Lechner david@lechnology.com
No need for a public header file for the command macros. Just include the necessary ones in the driver.
Also use the MIPI_DCS_PIXEL_FMT_16BIT macro.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/mi0283qt.c | 28 ++++++++++++++++++-- include/drm/tinydrm/ili9341.h | 54 -------------------------------------- 2 files changed, 26 insertions(+), 56 deletions(-) delete mode 100644 include/drm/tinydrm/ili9341.h
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 45f02b6052b1..c69a4d958f24 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -18,11 +18,35 @@
#include <drm/drm_fb_helper.h> #include <drm/drm_modeset_helper.h> -#include <drm/tinydrm/ili9341.h> #include <drm/tinydrm/mipi-dbi.h> #include <drm/tinydrm/tinydrm-helpers.h> #include <video/mipi_display.h>
+#define ILI9341_FRMCTR1 0xb1 +#define ILI9341_DISCTRL 0xb6 +#define ILI9341_ETMOD 0xb7 + +#define ILI9341_PWCTRL1 0xc0 +#define ILI9341_PWCTRL2 0xc1 +#define ILI9341_VMCTRL1 0xc5 +#define ILI9341_VMCTRL2 0xc7 +#define ILI9341_PWCTRLA 0xcb +#define ILI9341_PWCTRLB 0xcf + +#define ILI9341_PGAMCTRL 0xe0 +#define ILI9341_NGAMCTRL 0xe1 +#define ILI9341_DTCTRLA 0xe8 +#define ILI9341_DTCTRLB 0xea +#define ILI9341_PWRSEQ 0xed + +#define ILI9341_EN3GAM 0xf2 +#define ILI9341_PUMPCTRL 0xf7 + +#define ILI9341_MADCTL_BGR BIT(3) +#define ILI9341_MADCTL_MV BIT(5) +#define ILI9341_MADCTL_MX BIT(6) +#define ILI9341_MADCTL_MY BIT(7) + static int mi0283qt_init(struct mipi_dbi *mipi) { struct tinydrm_device *tdev = &mipi->tinydrm; @@ -69,7 +93,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi) mipi_dbi_command(mipi, ILI9341_VMCTRL2, 0xbe);
/* Memory Access Control */ - mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, 0x55); + mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
switch (mipi->rotation) { default: diff --git a/include/drm/tinydrm/ili9341.h b/include/drm/tinydrm/ili9341.h deleted file mode 100644 index 807a09f43cad..000000000000 --- a/include/drm/tinydrm/ili9341.h +++ /dev/null @@ -1,54 +0,0 @@ -/* - * ILI9341 LCD controller - * - * Copyright 2016 Noralf Trønnes - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - */ - -#ifndef __LINUX_ILI9341_H -#define __LINUX_ILI9341_H - -#define ILI9341_FRMCTR1 0xb1 -#define ILI9341_FRMCTR2 0xb2 -#define ILI9341_FRMCTR3 0xb3 -#define ILI9341_INVTR 0xb4 -#define ILI9341_PRCTR 0xb5 -#define ILI9341_DISCTRL 0xb6 -#define ILI9341_ETMOD 0xb7 - -#define ILI9341_PWCTRL1 0xc0 -#define ILI9341_PWCTRL2 0xc1 -#define ILI9341_VMCTRL1 0xc5 -#define ILI9341_VMCTRL2 0xc7 -#define ILI9341_PWCTRLA 0xcb -#define ILI9341_PWCTRLB 0xcf - -#define ILI9341_RDID1 0xda -#define ILI9341_RDID2 0xdb -#define ILI9341_RDID3 0xdc -#define ILI9341_RDID4 0xd3 - -#define ILI9341_PGAMCTRL 0xe0 -#define ILI9341_NGAMCTRL 0xe1 -#define ILI9341_DGAMCTRL1 0xe2 -#define ILI9341_DGAMCTRL2 0xe3 -#define ILI9341_DTCTRLA 0xe8 -#define ILI9341_DTCTRLB 0xea -#define ILI9341_PWRSEQ 0xed - -#define ILI9341_EN3GAM 0xf2 -#define ILI9341_IFCTRL 0xf6 -#define ILI9341_PUMPCTRL 0xf7 - -#define ILI9341_MADCTL_MH BIT(2) -#define ILI9341_MADCTL_BGR BIT(3) -#define ILI9341_MADCTL_ML BIT(4) -#define ILI9341_MADCTL_MV BIT(5) -#define ILI9341_MADCTL_MX BIT(6) -#define ILI9341_MADCTL_MY BIT(7) - -#endif /* __LINUX_ILI9341_H */
On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
No need for a public header file for the command macros. Just include the necessary ones in the driver.
Also use the MIPI_DCS_PIXEL_FMT_16BIT macro.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
Reviewed-by: David Lechner david@lechnology.com
Make it clear that the printed value is the error and not the command value itself.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/mi0283qt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index c69a4d958f24..5994140f1e1e 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -58,7 +58,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
ret = regulator_enable(mipi->regulator); if (ret) { - DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret); + DRM_DEV_ERROR(dev, "Failed to enable regulator: %d\n", ret); return ret; }
@@ -69,7 +69,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi) mipi_dbi_hw_reset(mipi); ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET); if (ret) { - DRM_DEV_ERROR(dev, "Error sending command %d\n", ret); + DRM_DEV_ERROR(dev, "Error sending command: %d\n", ret); regulator_disable(mipi->regulator); return ret; }
On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
Make it clear that the printed value is the error and not the command value itself.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/mi0283qt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index c69a4d958f24..5994140f1e1e 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -58,7 +58,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi)
ret = regulator_enable(mipi->regulator); if (ret) {
DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
DRM_DEV_ERROR(dev, "Failed to enable regulator: %d\n", ret);
It is more common to use parenthesis around the error value, so that would be more clear to me.
+ DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret);
return ret;}
@@ -69,7 +69,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi) mipi_dbi_hw_reset(mipi); ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET); if (ret) {
DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
DRM_DEV_ERROR(dev, "Error sending command: %d\n", ret);
ditto
regulator_disable(mipi->regulator); return ret;}
With that change:
Reviewed-by: David Lechner david@lechnology.com
It's better to leave power handling and controller init to the modesetting machinery using the simple pipe .enable and .disable callbacks.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------ drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++----- 2 files changed, 19 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 5994140f1e1e..8318a60e584c 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -47,9 +47,11 @@ #define ILI9341_MADCTL_MX BIT(6) #define ILI9341_MADCTL_MY BIT(7)
-static int mi0283qt_init(struct mipi_dbi *mipi) +static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, + struct drm_crtc_state *crtc_state) { - struct tinydrm_device *tdev = &mipi->tinydrm; + struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); + struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct device *dev = tdev->drm->dev; u8 addr_mode; int ret; @@ -59,19 +61,18 @@ static int mi0283qt_init(struct mipi_dbi *mipi) ret = regulator_enable(mipi->regulator); if (ret) { DRM_DEV_ERROR(dev, "Failed to enable regulator: %d\n", ret); - return ret; + return; }
- /* Avoid flicker by skipping setup if the bootloader has done it */ if (mipi_dbi_display_is_on(mipi)) - return 0; + goto out_enable;
mipi_dbi_hw_reset(mipi); ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET); if (ret) { DRM_DEV_ERROR(dev, "Error sending command: %d\n", ret); regulator_disable(mipi->regulator); - return ret; + return; }
msleep(20); @@ -137,19 +138,12 @@ static int mi0283qt_init(struct mipi_dbi *mipi) mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON); msleep(100);
- return 0; -} - -static void mi0283qt_fini(void *data) -{ - struct mipi_dbi *mipi = data; - - DRM_DEBUG_KMS("\n"); - regulator_disable(mipi->regulator); +out_enable: + mipi_dbi_pipe_enable(pipe, crtc_state); }
static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = { - .enable = mipi_dbi_pipe_enable, + .enable = mi0283qt_enable, .disable = mipi_dbi_pipe_disable, .update = tinydrm_display_pipe_update, .prepare_fb = tinydrm_display_pipe_prepare_fb, @@ -230,17 +224,6 @@ static int mi0283qt_probe(struct spi_device *spi) if (ret) return ret;
- ret = mi0283qt_init(mipi); - if (ret) - return ret; - - /* use devres to fini after drm unregister (drv->remove is before) */ - ret = devm_add_action(dev, mi0283qt_fini, mipi); - if (ret) { - mi0283qt_fini(mipi); - return ret; - } - spi_set_drvdata(spi, mipi);
return devm_tinydrm_register(&mipi->tinydrm); @@ -256,25 +239,13 @@ static void mi0283qt_shutdown(struct spi_device *spi) static int __maybe_unused mi0283qt_pm_suspend(struct device *dev) { struct mipi_dbi *mipi = dev_get_drvdata(dev); - int ret;
- ret = drm_mode_config_helper_suspend(mipi->tinydrm.drm); - if (ret) - return ret; - - mi0283qt_fini(mipi); - - return 0; + return drm_mode_config_helper_suspend(mipi->tinydrm.drm); }
static int __maybe_unused mi0283qt_pm_resume(struct device *dev) { struct mipi_dbi *mipi = dev_get_drvdata(dev); - int ret; - - ret = mi0283qt_init(mipi); - if (ret) - return ret;
drm_mode_config_helper_resume(mipi->tinydrm.drm);
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index aa6b6ce56891..5b5ed68d8994 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -275,8 +275,9 @@ static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = { * @pipe: Display pipe * @crtc_state: CRTC state * - * This function enables backlight. Drivers can use this as their - * &drm_simple_display_pipe_funcs->enable callback. + * This function flushes the whole framebuffer and enables the backlight. + * Drivers can use this in their &drm_simple_display_pipe_funcs->enable + * callback. */ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state) @@ -285,8 +286,6 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct drm_framebuffer *fb = pipe->plane.fb;
- DRM_DEBUG_KMS("\n"); - mipi->enabled = true; if (fb) fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); @@ -316,8 +315,8 @@ static void mipi_dbi_blank(struct mipi_dbi *mipi) * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper * @pipe: Display pipe * - * This function disables backlight if present or if not the - * display memory is blanked. Drivers can use this as their + * This function disables backlight if present, if not the display memory is + * blanked. The regulator is disabled if in use. Drivers can use this as their * &drm_simple_display_pipe_funcs->disable callback. */ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) @@ -333,6 +332,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) tinydrm_disable_backlight(mipi->backlight); else mipi_dbi_blank(mipi); + + if (mipi->regulator) + regulator_disable(mipi->regulator); } EXPORT_SYMBOL(mipi_dbi_pipe_disable);
On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
It's better to leave power handling and controller init to the modesetting machinery using the simple pipe .enable and .disable callbacks.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------ drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++----- 2 files changed, 19 insertions(+), 46 deletions(-)
<snip>
@@ -316,8 +315,8 @@ static void mipi_dbi_blank(struct mipi_dbi *mipi)
- mipi_dbi_pipe_disable - MIPI DBI pipe disable helper
- @pipe: Display pipe
- This function disables backlight if present or if not the
- display memory is blanked. Drivers can use this as their
- This function disables backlight if present, if not the display memory is
*/ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
- blanked. The regulator is disabled if in use. Drivers can use this as their
- &drm_simple_display_pipe_funcs->disable callback.
@@ -333,6 +332,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) tinydrm_disable_backlight(mipi->backlight); else mipi_dbi_blank(mipi);
- if (mipi->regulator)
regulator_disable(mipi->regulator);
If a display physically has a backlight, but it is not controllable (i.e. mipi->backlight == NULL) and you disable the regulator, would that not cause the display to be all white instead of blanked?
Also, even if this is OK, it seems like you should call regulator_enable() in mipi_dbi_pipe_enable() to keep things balanced in the helper functions.
Den 05.01.2018 19.59, skrev David Lechner:
On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
It's better to leave power handling and controller init to the modesetting machinery using the simple pipe .enable and .disable callbacks.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------ drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++----- 2 files changed, 19 insertions(+), 46 deletions(-)
<snip>
@@ -316,8 +315,8 @@ static void mipi_dbi_blank(struct mipi_dbi *mipi) * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper * @pipe: Display pipe *
- This function disables backlight if present or if not the
- display memory is blanked. Drivers can use this as their
- This function disables backlight if present, if not the display
memory is
- blanked. The regulator is disabled if in use. Drivers can use
this as their * &drm_simple_display_pipe_funcs->disable callback. */ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) @@ -333,6 +332,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) tinydrm_disable_backlight(mipi->backlight); else mipi_dbi_blank(mipi);
+ if (mipi->regulator) + regulator_disable(mipi->regulator); } EXPORT_SYMBOL(mipi_dbi_pipe_disable);
If a display physically has a backlight, but it is not controllable (i.e. mipi->backlight == NULL) and you disable the regulator, would that not cause the display to be all white instead of blanked?
Yes, if the regulator doesn't control the backlight. But on these simple displays I assume that a regulator would control the whole display including the backlight. If we get a display with a separate backlight regulator, I think it's better to treat that as the exception rather than the other way around.
Also, even if this is OK, it seems like you should call regulator_enable() in mipi_dbi_pipe_enable() to keep things balanced in the helper functions.
Yes, I wasn't entirely happy with this. The regulator has to be enabled before the controller is initialized, so it can't happen in this _enable helper. I thought about having mipi_dbi_pipe_enable_begin/end functions, but I'm not sure I like that either. I prefer something more descriptive.
Maybe we should drop the pipe enable helper, since it's not very likely that anyone can use it directly as an .enable function.
What do you think about this approach:
/** * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset * @mipi: MIPI DBI structure * * This function enables the regulator if used and if the display is off, it * does a hardware and software reset. If mipi_dbi_display_is_on() determines * that the display is on, no reset is performed. * * Returns: * Zero if the controller was reset, 1 if the display was already on, or a * negative error code. */ int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi) { struct device *dev = mipi->tinydrm.drm->dev; int ret;
if (mipi->regulator) { ret = regulator_enable(mipi->regulator); if (ret) { DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret); return ret; } }
if (mipi_dbi_display_is_on(mipi)) return 1;
mipi_dbi_hw_reset(mipi); ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET); if (ret) { DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret); regulator_disable(mipi->regulator); return ret; }
return 0; } EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
/** * mipi_dbi_enable_flush - MIPI DBI enable helper * @mipi: MIPI DBI structure * * This function sets &mipi_dbi->enabled, flushes the whole framebuffer and * enables the backlight. Drivers can use this in their * &drm_simple_display_pipe_funcs->enable callback. */ void mipi_dbi_enable_flush(struct mipi_dbi *mipi) { struct drm_framebuffer *fb = mipi->tdev.pipe->plane.fb;
mipi->enabled = true; if (fb) fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
tinydrm_enable_backlight(mipi->backlight); } EXPORT_SYMBOL(mipi_dbi_enable_flush);
static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state) { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct device *dev = tdev->drm->dev;
ret = mipi_dbi_poweron_conditional_reset(mipi); if (ret < 0) return; if (ret > 0) goto out_enable;
/* initialize controller */
out_enable: mipi_dbi_enable_flush(mipi); }
Noralf.
On 01/06/2018 06:45 AM, Noralf Trønnes wrote:
Den 05.01.2018 19.59, skrev David Lechner:
On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
It's better to leave power handling and controller init to the modesetting machinery using the simple pipe .enable and .disable callbacks.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------ drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++----- 2 files changed, 19 insertions(+), 46 deletions(-)
<snip>
@@ -316,8 +315,8 @@ static void mipi_dbi_blank(struct mipi_dbi *mipi) * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper * @pipe: Display pipe *
- This function disables backlight if present or if not the
- display memory is blanked. Drivers can use this as their
- This function disables backlight if present, if not the display memory is
- blanked. The regulator is disabled if in use. Drivers can use this as their
* &drm_simple_display_pipe_funcs->disable callback. */ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) @@ -333,6 +332,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) tinydrm_disable_backlight(mipi->backlight); else mipi_dbi_blank(mipi);
+ if (mipi->regulator) + regulator_disable(mipi->regulator); } EXPORT_SYMBOL(mipi_dbi_pipe_disable);
If a display physically has a backlight, but it is not controllable (i.e. mipi->backlight == NULL) and you disable the regulator, would that not cause the display to be all white instead of blanked?
Yes, if the regulator doesn't control the backlight. But on these simple displays I assume that a regulator would control the whole display including the backlight. If we get a display with a separate backlight regulator, I think it's better to treat that as the exception rather than the other way around.
Makes sense. All of the displays I have that have a controllable backlight don't have a regulator for the display itself, so they won't be affected anyway.
Also, even if this is OK, it seems like you should call regulator_enable() in mipi_dbi_pipe_enable() to keep things balanced in the helper functions.
Yes, I wasn't entirely happy with this. The regulator has to be enabled before the controller is initialized, so it can't happen in this _enable helper. I thought about having mipi_dbi_pipe_enable_begin/end functions, but I'm not sure I like that either. I prefer something more descriptive.
Maybe we should drop the pipe enable helper, since it's not very likely that anyone can use it directly as an .enable function.
What do you think about this approach:
It looks OK to me. Although I am wondering about the conditional reset. If the display is already on, how do we know it is configured correctly?
/** * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset * @mipi: MIPI DBI structure * * This function enables the regulator if used and if the display is off, it * does a hardware and software reset. If mipi_dbi_display_is_on() determines * that the display is on, no reset is performed. * * Returns: * Zero if the controller was reset, 1 if the display was already on, or a * negative error code. */ int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi) { struct device *dev = mipi->tinydrm.drm->dev; int ret;
if (mipi->regulator) { ret = regulator_enable(mipi->regulator); if (ret) { DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret); return ret; } }
if (mipi_dbi_display_is_on(mipi)) return 1;
mipi_dbi_hw_reset(mipi); ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET); if (ret) { DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret); regulator_disable(mipi->regulator); return ret; }
return 0; } EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
/** * mipi_dbi_enable_flush - MIPI DBI enable helper * @mipi: MIPI DBI structure * * This function sets &mipi_dbi->enabled, flushes the whole framebuffer and * enables the backlight. Drivers can use this in their * &drm_simple_display_pipe_funcs->enable callback. */ void mipi_dbi_enable_flush(struct mipi_dbi *mipi) { struct drm_framebuffer *fb = mipi->tdev.pipe->plane.fb;
mipi->enabled = true; if (fb) fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
tinydrm_enable_backlight(mipi->backlight); } EXPORT_SYMBOL(mipi_dbi_enable_flush);
static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state) { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct device *dev = tdev->drm->dev;
ret = mipi_dbi_poweron_conditional_reset(mipi); if (ret < 0) return; if (ret > 0) goto out_enable;
/* initialize controller */
out_enable: mipi_dbi_enable_flush(mipi); }
Noralf.
Den 06.01.2018 19.10, skrev David Lechner:
On 01/06/2018 06:45 AM, Noralf Trønnes wrote:
Den 05.01.2018 19.59, skrev David Lechner:
On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
It's better to leave power handling and controller init to the modesetting machinery using the simple pipe .enable and .disable callbacks.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/mi0283qt.c | 51 ++++++++------------------------------ drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++----- 2 files changed, 19 insertions(+), 46 deletions(-)
<snip>
@@ -316,8 +315,8 @@ static void mipi_dbi_blank(struct mipi_dbi *mipi) * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper * @pipe: Display pipe *
- This function disables backlight if present or if not the
- display memory is blanked. Drivers can use this as their
- This function disables backlight if present, if not the display
memory is
- blanked. The regulator is disabled if in use. Drivers can use
this as their * &drm_simple_display_pipe_funcs->disable callback. */ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) @@ -333,6 +332,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) tinydrm_disable_backlight(mipi->backlight); else mipi_dbi_blank(mipi);
+ if (mipi->regulator) + regulator_disable(mipi->regulator); } EXPORT_SYMBOL(mipi_dbi_pipe_disable);
If a display physically has a backlight, but it is not controllable (i.e. mipi->backlight == NULL) and you disable the regulator, would that not cause the display to be all white instead of blanked?
Yes, if the regulator doesn't control the backlight. But on these simple displays I assume that a regulator would control the whole display including the backlight. If we get a display with a separate backlight regulator, I think it's better to treat that as the exception rather than the other way around.
Makes sense. All of the displays I have that have a controllable backlight don't have a regulator for the display itself, so they won't be affected anyway.
Also, even if this is OK, it seems like you should call regulator_enable() in mipi_dbi_pipe_enable() to keep things balanced in the helper functions.
Yes, I wasn't entirely happy with this. The regulator has to be enabled before the controller is initialized, so it can't happen in this _enable helper. I thought about having mipi_dbi_pipe_enable_begin/end functions, but I'm not sure I like that either. I prefer something more descriptive.
Maybe we should drop the pipe enable helper, since it's not very likely that anyone can use it directly as an .enable function.
What do you think about this approach:
It looks OK to me. Although I am wondering about the conditional reset. If the display is already on, how do we know it is configured correctly?
The MIPI DCS spec says that the reset value of get_power_mode 0Ah is 08h. That's 0b00001000 which differs from what we call ON: 0bX00111XX.
If mipi->read_commands isn't NULL and the MISO pin isn't connected we will read all 1's or all 0's (unless it's floating), which isn't a problem since it will return false in that case.
The only way to get ON if the driver didn't do it, is if the bootloader did it. In that case we trust the bootloader.
This speeds up subsequent .enable calls, since we can avoid the msleeps.
MIPI_DCS_GET_POWER_MODE = 0x0A,
#define DCS_POWER_MODE_DISPLAY BIT(2) #define DCS_POWER_MODE_DISPLAY_NORMAL_MODE BIT(3) #define DCS_POWER_MODE_SLEEP_MODE BIT(4) #define DCS_POWER_MODE_PARTIAL_MODE BIT(5) #define DCS_POWER_MODE_IDLE_MODE BIT(6) #define DCS_POWER_MODE_RESERVED_MASK (BIT(0) | BIT(1) | BIT(7))
bool mipi_dbi_display_is_on(struct mipi_dbi *mipi) { u8 val;
if (mipi_dbi_command_read(mipi, MIPI_DCS_GET_POWER_MODE, &val)) return false;
val &= ~DCS_POWER_MODE_RESERVED_MASK;
if (val != (DCS_POWER_MODE_DISPLAY | DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE)) return false;
DRM_DEBUG_DRIVER("Display is ON\n");
return true; }
Noralf.
/** * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset * @mipi: MIPI DBI structure * * This function enables the regulator if used and if the display is off, it * does a hardware and software reset. If mipi_dbi_display_is_on() determines * that the display is on, no reset is performed. * * Returns: * Zero if the controller was reset, 1 if the display was already on, or a * negative error code. */ int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi) { struct device *dev = mipi->tinydrm.drm->dev; int ret;
if (mipi->regulator) { ret = regulator_enable(mipi->regulator); if (ret) { DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", ret); return ret; } }
if (mipi_dbi_display_is_on(mipi)) return 1;
mipi_dbi_hw_reset(mipi); ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET); if (ret) { DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret); regulator_disable(mipi->regulator); return ret; }
return 0; } EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
/** * mipi_dbi_enable_flush - MIPI DBI enable helper * @mipi: MIPI DBI structure * * This function sets &mipi_dbi->enabled, flushes the whole framebuffer and * enables the backlight. Drivers can use this in their * &drm_simple_display_pipe_funcs->enable callback. */ void mipi_dbi_enable_flush(struct mipi_dbi *mipi) { struct drm_framebuffer *fb = mipi->tdev.pipe->plane.fb;
mipi->enabled = true; if (fb) fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
tinydrm_enable_backlight(mipi->backlight); } EXPORT_SYMBOL(mipi_dbi_enable_flush);
static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state) { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct device *dev = tdev->drm->dev;
ret = mipi_dbi_poweron_conditional_reset(mipi); if (ret < 0) return; if (ret > 0) goto out_enable;
/* initialize controller */
out_enable: mipi_dbi_enable_flush(mipi); }
Noralf.
Embed the mode in tinydrm_connector instead of doing an devm_ allocation. Remove unnecessary use of ret variable at the end of tinydrm_display_pipe_init().
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------ 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index f41fc506ff87..dadd26d53216 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -15,7 +15,7 @@
struct tinydrm_connector { struct drm_connector base; - const struct drm_display_mode *mode; + struct drm_display_mode mode; };
static inline struct tinydrm_connector * @@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector) struct tinydrm_connector *tconn = to_tinydrm_connector(connector); struct drm_display_mode *mode;
- mode = drm_mode_duplicate(connector->dev, tconn->mode); + mode = drm_mode_duplicate(connector->dev, &tconn->mode); if (!mode) { DRM_ERROR("Failed to duplicate mode\n"); return 0; @@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, if (!tconn) return ERR_PTR(-ENOMEM);
- tconn->mode = mode; + tconn->mode = *mode; connector = &tconn->base;
drm_connector_helper_add(connector, &tinydrm_connector_hfuncs); @@ -199,35 +199,27 @@ tinydrm_display_pipe_init(struct tinydrm_device *tdev, unsigned int rotation) { struct drm_device *drm = tdev->drm; - struct drm_display_mode *mode_copy; + struct drm_display_mode mode_copy; struct drm_connector *connector; int ret;
- mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), GFP_KERNEL); - if (!mode_copy) - return -ENOMEM; - - *mode_copy = *mode; - ret = tinydrm_rotate_mode(mode_copy, rotation); + mode_copy = *mode; + ret = tinydrm_rotate_mode(&mode_copy, rotation); if (ret) { DRM_ERROR("Illegal rotation value %u\n", rotation); return -EINVAL; }
- drm->mode_config.min_width = mode_copy->hdisplay; - drm->mode_config.max_width = mode_copy->hdisplay; - drm->mode_config.min_height = mode_copy->vdisplay; - drm->mode_config.max_height = mode_copy->vdisplay; + drm->mode_config.min_width = mode_copy.hdisplay; + drm->mode_config.max_width = mode_copy.hdisplay; + drm->mode_config.min_height = mode_copy.vdisplay; + drm->mode_config.max_height = mode_copy.vdisplay;
- connector = tinydrm_connector_create(drm, mode_copy, connector_type); + connector = tinydrm_connector_create(drm, &mode_copy, connector_type); if (IS_ERR(connector)) return PTR_ERR(connector);
- ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, - format_count, NULL, connector); - if (ret) - return ret; - - return 0; + return drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, + format_count, NULL, connector); } EXPORT_SYMBOL(tinydrm_display_pipe_init);
On 01/05/2018 10:56 AM, Noralf Trønnes wrote:
Embed the mode in tinydrm_connector instead of doing an devm_ allocation. Remove unnecessary use of ret variable at the end of tinydrm_display_pipe_init().
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------ 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index f41fc506ff87..dadd26d53216 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -15,7 +15,7 @@
struct tinydrm_connector { struct drm_connector base;
- const struct drm_display_mode *mode;
struct drm_display_mode mode; };
static inline struct tinydrm_connector *
@@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector) struct tinydrm_connector *tconn = to_tinydrm_connector(connector); struct drm_display_mode *mode;
- mode = drm_mode_duplicate(connector->dev, tconn->mode);
- mode = drm_mode_duplicate(connector->dev, &tconn->mode); if (!mode) { DRM_ERROR("Failed to duplicate mode\n"); return 0;
@@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, if (!tconn) return ERR_PTR(-ENOMEM);
- tconn->mode = mode;
- tconn->mode = *mode;
I see there is a special drm_mode_copy() function, so I am wondering if this is safe.
connector = &tconn->base;
drm_connector_helper_add(connector, &tinydrm_connector_hfuncs); @@ -199,35 +199,27 @@ tinydrm_display_pipe_init(struct tinydrm_device *tdev, unsigned int rotation) { struct drm_device *drm = tdev->drm;
- struct drm_display_mode *mode_copy;
- struct drm_display_mode mode_copy; struct drm_connector *connector; int ret;
- mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), GFP_KERNEL);
- if (!mode_copy)
return -ENOMEM;- *mode_copy = *mode;
- ret = tinydrm_rotate_mode(mode_copy, rotation);
- mode_copy = *mode;
Same here.
- ret = tinydrm_rotate_mode(&mode_copy, rotation); if (ret) { DRM_ERROR("Illegal rotation value %u\n", rotation); return -EINVAL; }
- drm->mode_config.min_width = mode_copy->hdisplay;
- drm->mode_config.max_width = mode_copy->hdisplay;
- drm->mode_config.min_height = mode_copy->vdisplay;
- drm->mode_config.max_height = mode_copy->vdisplay;
- drm->mode_config.min_width = mode_copy.hdisplay;
- drm->mode_config.max_width = mode_copy.hdisplay;
- drm->mode_config.min_height = mode_copy.vdisplay;
- drm->mode_config.max_height = mode_copy.vdisplay;
- connector = tinydrm_connector_create(drm, mode_copy, connector_type);
- connector = tinydrm_connector_create(drm, &mode_copy, connector_type); if (IS_ERR(connector)) return PTR_ERR(connector);
- ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats,
format_count, NULL, connector);- if (ret)
return ret;- return 0;
- return drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats,
format_count, NULL, connector);
Den 05.01.2018 20.14, skrev David Lechner:
On 01/05/2018 10:56 AM, Noralf Trønnes wrote:
Embed the mode in tinydrm_connector instead of doing an devm_ allocation. Remove unnecessary use of ret variable at the end of tinydrm_display_pipe_init().
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------ 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index f41fc506ff87..dadd26d53216 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -15,7 +15,7 @@ struct tinydrm_connector { struct drm_connector base; - const struct drm_display_mode *mode; + struct drm_display_mode mode; }; static inline struct tinydrm_connector * @@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector) struct tinydrm_connector *tconn = to_tinydrm_connector(connector); struct drm_display_mode *mode; - mode = drm_mode_duplicate(connector->dev, tconn->mode); + mode = drm_mode_duplicate(connector->dev, &tconn->mode); if (!mode) { DRM_ERROR("Failed to duplicate mode\n"); return 0; @@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, if (!tconn) return ERR_PTR(-ENOMEM); - tconn->mode = mode; + tconn->mode = *mode;
I see there is a special drm_mode_copy() function, so I am wondering if this is safe.
It is safe, the underlying drm_mode_object isn't initialized/registered. So to me an assignment like this makes it clear that it is so, but Laurent also asked about this, so maybe I'm the odd one here. I'll switch to drm_mode_copy().
Noralf.
connector = &tconn->base; drm_connector_helper_add(connector, &tinydrm_connector_hfuncs); @@ -199,35 +199,27 @@ tinydrm_display_pipe_init(struct tinydrm_device *tdev, unsigned int rotation) { struct drm_device *drm = tdev->drm; - struct drm_display_mode *mode_copy; + struct drm_display_mode mode_copy; struct drm_connector *connector; int ret; - mode_copy = devm_kmalloc(drm->dev, sizeof(*mode_copy), GFP_KERNEL); - if (!mode_copy) - return -ENOMEM;
- *mode_copy = *mode; - ret = tinydrm_rotate_mode(mode_copy, rotation); + mode_copy = *mode;
Same here.
+ ret = tinydrm_rotate_mode(&mode_copy, rotation); if (ret) { DRM_ERROR("Illegal rotation value %u\n", rotation); return -EINVAL; } - drm->mode_config.min_width = mode_copy->hdisplay; - drm->mode_config.max_width = mode_copy->hdisplay; - drm->mode_config.min_height = mode_copy->vdisplay; - drm->mode_config.max_height = mode_copy->vdisplay; + drm->mode_config.min_width = mode_copy.hdisplay; + drm->mode_config.max_width = mode_copy.hdisplay; + drm->mode_config.min_height = mode_copy.vdisplay; + drm->mode_config.max_height = mode_copy.vdisplay; - connector = tinydrm_connector_create(drm, mode_copy, connector_type); + connector = tinydrm_connector_create(drm, &mode_copy, connector_type); if (IS_ERR(connector)) return PTR_ERR(connector); - ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, - format_count, NULL, connector); - if (ret) - return ret;
- return 0; + return drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, + format_count, NULL, connector); } EXPORT_SYMBOL(tinydrm_display_pipe_init);
On 01/06/2018 06:49 AM, Noralf Trønnes wrote:
Den 05.01.2018 20.14, skrev David Lechner:
On 01/05/2018 10:56 AM, Noralf Trønnes wrote:
Embed the mode in tinydrm_connector instead of doing an devm_ allocation. Remove unnecessary use of ret variable at the end of tinydrm_display_pipe_init().
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 34 +++++++++++------------------ 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index f41fc506ff87..dadd26d53216 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -15,7 +15,7 @@ struct tinydrm_connector { struct drm_connector base; - const struct drm_display_mode *mode; + struct drm_display_mode mode; }; static inline struct tinydrm_connector * @@ -29,7 +29,7 @@ static int tinydrm_connector_get_modes(struct drm_connector *connector) struct tinydrm_connector *tconn = to_tinydrm_connector(connector); struct drm_display_mode *mode; - mode = drm_mode_duplicate(connector->dev, tconn->mode); + mode = drm_mode_duplicate(connector->dev, &tconn->mode); if (!mode) { DRM_ERROR("Failed to duplicate mode\n"); return 0; @@ -92,7 +92,7 @@ tinydrm_connector_create(struct drm_device *drm, if (!tconn) return ERR_PTR(-ENOMEM); - tconn->mode = mode; + tconn->mode = *mode;
I see there is a special drm_mode_copy() function, so I am wondering if this is safe.
It is safe, the underlying drm_mode_object isn't initialized/registered. So to me an assignment like this makes it clear that it is so, but Laurent also asked about this, so maybe I'm the odd one here. I'll switch to drm_mode_copy().
I think a comment to this effect would be enough.
dri-devel@lists.freedesktop.org
-
David Lechner -
Noralf Trønnes