* [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags
@ 2018-09-22 12:15 Laurent Pinchart
2018-09-22 12:15 ` [RFC/PATCH 1/2] drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros Laurent Pinchart
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Laurent Pinchart @ 2018-09-22 12:15 UTC (permalink / raw)
To: dri-devel; +Cc: Maxime Ripard, Thierry Reding
Hello,
This patch series attemps at clarifying usage of the
DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags. It results from a discussion
on the mailing list available at [1].
The problem being discussed was confusion around how the
DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE flags
could be interpreted (and are interpreted now by drivers). Patch 1/2
introduces new, more explicit flags, and explains the rationale. Patch
2/2 then updates the drivers to use the new flags.
[1] https://www.spinics.net/lists/arm-kernel/msg677079.html
Laurent Pinchart (2):
drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros
drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags
drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++---
drivers/gpu/drm/drm_modes.c | 8 ++++----
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +-
drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +-
drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++---
drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +-
.../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +-
.../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +-
.../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +-
.../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +-
.../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +-
.../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +-
drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +-
drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +-
drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++--
drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++--
drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++--
drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +-
drivers/gpu/drm/panel/panel-simple.c | 20 ++++++++++----------
drivers/gpu/drm/pl111/pl111_display.c | 2 +-
drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
drivers/gpu/drm/tve200/tve200_display.c | 3 ++-
include/drm/drm_bridge.h | 8 ++++----
include/drm/drm_connector.h | 20 ++++++++++++++++++--
24 files changed, 65 insertions(+), 48 deletions(-)
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* [RFC/PATCH 1/2] drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros 2018-09-22 12:15 [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Laurent Pinchart @ 2018-09-22 12:15 ` Laurent Pinchart 2018-09-22 12:15 ` [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags Laurent Pinchart ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2018-09-22 12:15 UTC (permalink / raw) To: dri-devel; +Cc: Maxime Ripard, Thierry Reding The DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE macros define on which pixel clock edge data signals are driven. They are however used in some drivers to define on which pixel clock edge data signals are sampled, which should usually (but not always) be the opposite edge of the driving edge. This creates confusion. Create four new macros explicitly stating the driving and sampling edge in their name to remove the confusing. The driving macros are defined as the opposite of the sampling macros to made code simpler based on the assumption that the driving and sampling edges are opposite. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- include/drm/drm_connector.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 91a877fa00cb..b0accf804d1c 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -283,10 +283,26 @@ struct drm_display_info { #define DRM_BUS_FLAG_DE_LOW (1<<0) #define DRM_BUS_FLAG_DE_HIGH (1<<1) -/* drive data on pos. edge */ + +/* + * Don't use those two flags directly, use the DRM_BUS_FLAG_PIXDATA_DRIVE_* + * and DRM_BUS_FLAG_PIXDATA_SAMPLE_* variants to qualify the flags explicitly. + * The DRM_BUS_FLAG_PIXDATA_SAMPLE_* flags are defined as the opposite of the + * DRM_BUS_FLAG_PIXDATA_DRIVE_* flags to make code simpler, as signals are + * usually to be sampled on the opposite edge of the driving edge. + */ #define DRM_BUS_FLAG_PIXDATA_POSEDGE (1<<2) -/* drive data on neg. edge */ #define DRM_BUS_FLAG_PIXDATA_NEGEDGE (1<<3) + +/* Drive data on rising edge */ +#define DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE DRM_BUS_FLAG_PIXDATA_POSEDGE +/* Drive data on falling edge */ +#define DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE DRM_BUS_FLAG_PIXDATA_NEGEDGE +/* Sample data on rising edge */ +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE DRM_BUS_FLAG_PIXDATA_NEGEDGE +/* Sample data on falling edge */ +#define DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE DRM_BUS_FLAG_PIXDATA_POSEDGE + /* data is transmitted MSB to LSB on the bus */ #define DRM_BUS_FLAG_DATA_MSB_TO_LSB (1<<4) /* data is transmitted LSB to MSB on the bus */ -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags 2018-09-22 12:15 [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Laurent Pinchart 2018-09-22 12:15 ` [RFC/PATCH 1/2] drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros Laurent Pinchart @ 2018-09-22 12:15 ` Laurent Pinchart 2018-09-24 11:48 ` Thierry Reding 2018-09-24 11:54 ` [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Stefan Agner 2018-09-26 7:37 ` Linus Walleij 3 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2018-09-22 12:15 UTC (permalink / raw) To: dri-devel; +Cc: Maxime Ripard, Thierry Reding The DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags are deprecated in favour of the new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags. Replace them through the code. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- drivers/gpu/drm/drm_modes.c | 8 ++++---- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- drivers/gpu/drm/panel/panel-simple.c | 20 ++++++++++---------- drivers/gpu/drm/pl111/pl111_display.c | 2 +- drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- drivers/gpu/drm/tve200/tve200_display.c | 3 ++- include/drm/drm_bridge.h | 8 ++++---- 23 files changed, 47 insertions(+), 46 deletions(-) diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index 9b706789a341..7dc14c22f7db 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev) */ static const struct drm_bridge_timings default_dac_timings = { /* Timing specifications, datasheet page 7 */ - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, .setup_time_ps = 500, .hold_time_ps = 1500, }; @@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = { */ static const struct drm_bridge_timings ti_ths8134_dac_timings = { /* From timing diagram, datasheet page 9 */ - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, /* From datasheet, page 12 */ .setup_time_ps = 3000, /* I guess this means latched input */ @@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = { */ static const struct drm_bridge_timings ti_ths8135_dac_timings = { /* From timing diagram, datasheet page 14 */ - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, /* From datasheet, page 16 */ .setup_time_ps = 2000, .hold_time_ps = 500, diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 02db9ac82d7a..475bdc8a6420 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -662,17 +662,17 @@ EXPORT_SYMBOL_GPL(drm_display_mode_to_videomode); * @bus_flags: information about pixelclk, sync and DE polarity will be stored * here * - * Sets DRM_BUS_FLAG_DE_(LOW|HIGH), DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE and - * DISPLAY_FLAGS_SYNC_(POS|NEG)EDGE in @bus_flags according to DISPLAY_FLAGS + * Sets DRM_BUS_FLAG_DE_(LOW|HIGH), DRM_BUS_FLAG_PIXDATA_DRIVE_(POS|NEG)EDGE + * and DISPLAY_FLAGS_SYNC_(POS|NEG)EDGE in @bus_flags according to DISPLAY_FLAGS * found in @vm */ void drm_bus_flags_from_videomode(const struct videomode *vm, u32 *bus_flags) { *bus_flags = 0; if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE) - *bus_flags |= DRM_BUS_FLAG_PIXDATA_POSEDGE; + *bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; if (vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE) - *bus_flags |= DRM_BUS_FLAG_PIXDATA_NEGEDGE; + *bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE; if (vm->flags & DISPLAY_FLAGS_SYNC_POSEDGE) *bus_flags |= DRM_BUS_FLAG_SYNC_POSEDGE; diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c index 0e3752437e44..ba0d00f1a8b1 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c @@ -99,7 +99,7 @@ static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc) vsw = mode->vsync_end - mode->vsync_start; /* INV_PXCK as default (most display sample data on rising edge) */ - if (!(con->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)) + if (!(con->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)) pol |= DCU_SYN_POL_INV_PXCK; if (mode->flags & DRM_MODE_FLAG_NHSYNC) diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 7d4b710b837a..82c513fc0ae3 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -277,7 +277,7 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc) sig_cfg.enable_pol = !(imx_crtc_state->bus_flags & DRM_BUS_FLAG_DE_LOW); /* Default to driving pixel data on negative clock edges */ sig_cfg.clk_pol = !!(imx_crtc_state->bus_flags & - DRM_BUS_FLAG_PIXDATA_POSEDGE); + DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE); sig_cfg.bus_format = imx_crtc_state->bus_format; sig_cfg.v_to_h_sync = 0; sig_cfg.hsync_pin = imx_crtc_state->di_hsync_pin; diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c index 0abe77675b76..bbbf4b3df949 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c @@ -242,12 +242,12 @@ static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb) if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH; /* - * DRM_BUS_FLAG_PIXDATA_ defines are controller centric, + * DRM_BUS_FLAG_PIXDATA_DRIVE_ defines are controller centric, * controllers VDCTRL0_DOTCLK is display centric. * Drive on positive edge -> display samples on falling edge - * DRM_BUS_FLAG_PIXDATA_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING + * DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE -> VDCTRL0_DOTCLK_ACT_FALLING */ - if (bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE) + if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) vdctrl0 |= VDCTRL0_DOTCLK_ACT_FALLING; writel(vdctrl0, mxsfb->base + LCDC_VDCTRL0); diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c b/drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c index f1a748353279..0a8132c2352a 100644 --- a/drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c +++ b/drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c @@ -112,7 +112,7 @@ static int tfp410_probe(struct platform_device *pdev) dssdev->owner = THIS_MODULE; dssdev->of_ports = BIT(1) | BIT(0); dssdev->bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_POSEDGE - | DRM_BUS_FLAG_PIXDATA_POSEDGE; + | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; dssdev->next = omapdss_of_find_connected_device(pdev->dev.of_node, 1); if (IS_ERR(dssdev->next)) { diff --git a/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c b/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c index f6ef8ff964dd..b468ef158bc8 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c @@ -230,7 +230,7 @@ static int lb035q02_panel_spi_probe(struct spi_device *spi) * DATA needs to be driven on the FALLING edge */ dssdev->bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_NEGEDGE - | DRM_BUS_FLAG_PIXDATA_POSEDGE; + | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; omapdss_display_init(dssdev); omapdss_device_register(dssdev); diff --git a/drivers/gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c b/drivers/gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c index f445de6369f7..09796105796d 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c @@ -219,7 +219,7 @@ static int nec_8048_probe(struct spi_device *spi) dssdev->owner = THIS_MODULE; dssdev->of_ports = BIT(0); dssdev->bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_POSEDGE - | DRM_BUS_FLAG_PIXDATA_POSEDGE; + | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; omapdss_display_init(dssdev); omapdss_device_register(dssdev); diff --git a/drivers/gpu/drm/omapdrm/displays/panel-sharp-ls037v7dw01.c b/drivers/gpu/drm/omapdrm/displays/panel-sharp-ls037v7dw01.c index 64b1369cb274..63e6db2b9a69 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-sharp-ls037v7dw01.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-sharp-ls037v7dw01.c @@ -228,7 +228,7 @@ static int sharp_ls_probe(struct platform_device *pdev) * DATA needs to be driven on the FALLING edge */ dssdev->bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_NEGEDGE - | DRM_BUS_FLAG_PIXDATA_POSEDGE; + | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; omapdss_display_init(dssdev); omapdss_device_register(dssdev); diff --git a/drivers/gpu/drm/omapdrm/displays/panel-sony-acx565akm.c b/drivers/gpu/drm/omapdrm/displays/panel-sony-acx565akm.c index e04663856b31..0fb04ce024b1 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-sony-acx565akm.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-sony-acx565akm.c @@ -742,7 +742,7 @@ static int acx565akm_probe(struct spi_device *spi) dssdev->owner = THIS_MODULE; dssdev->of_ports = BIT(0); dssdev->bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_NEGEDGE - | DRM_BUS_FLAG_PIXDATA_POSEDGE; + | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; omapdss_display_init(dssdev); omapdss_device_register(dssdev); diff --git a/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c b/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c index 7ddc8c574a61..66a1a2d456b4 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c @@ -352,7 +352,7 @@ static int td028ttec1_panel_probe(struct spi_device *spi) * SYNC needs to be driven on the FALLING edge */ dssdev->bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_POSEDGE - | DRM_BUS_FLAG_PIXDATA_NEGEDGE; + | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE; omapdss_display_init(dssdev); omapdss_device_register(dssdev); diff --git a/drivers/gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c b/drivers/gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c index 8440fcb744d9..f5608e0e655e 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c @@ -450,7 +450,7 @@ static int tpo_td043_probe(struct spi_device *spi) * SYNC needs to be driven on the FALLING edge */ dssdev->bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_POSEDGE - | DRM_BUS_FLAG_PIXDATA_NEGEDGE; + | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE; omapdss_display_init(dssdev); omapdss_device_register(dssdev); diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c index 394c129cfb3b..06952e0cc3cc 100644 --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5135,7 +5135,7 @@ static int dsi_init_output(struct dsi_data *dsi) out->ops = &dsi_ops; out->owner = THIS_MODULE; out->of_ports = BIT(0); - out->bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE + out->bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE | DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_SYNC_NEGEDGE; diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c index b2fe2387037a..656e5838b40a 100644 --- a/drivers/gpu/drm/omapdrm/dss/sdi.c +++ b/drivers/gpu/drm/omapdrm/dss/sdi.c @@ -294,7 +294,7 @@ static int sdi_init_output(struct sdi_device *sdi) out->of_ports = BIT(1); out->ops = &sdi_ops; out->owner = THIS_MODULE; - out->bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE /* 15.5.9.1.2 */ + out->bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE /* 15.5.9.1.2 */ | DRM_BUS_FLAG_SYNC_POSEDGE; out->next = omapdss_of_find_connected_device(out->dev->of_node, 1); diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index 452e625f6ce3..e7e833da0ee3 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -88,9 +88,9 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder, if (!(vm.flags & (DISPLAY_FLAGS_PIXDATA_POSEDGE | DISPLAY_FLAGS_PIXDATA_NEGEDGE))) { - if (bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE) + if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) vm.flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE; - else if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) + else if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) vm.flags |= DISPLAY_FLAGS_PIXDATA_NEGEDGE; } diff --git a/drivers/gpu/drm/panel/panel-arm-versatile.c b/drivers/gpu/drm/panel/panel-arm-versatile.c index b428c4678106..078fa2c0eef8 100644 --- a/drivers/gpu/drm/panel/panel-arm-versatile.c +++ b/drivers/gpu/drm/panel/panel-arm-versatile.c @@ -191,7 +191,7 @@ static const struct versatile_panel_type versatile_panels[] = { .vrefresh = 390, .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC, }, - .bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, + .bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE, }, /* * Sanyo ALR252RGT 240x320 portrait display found on the @@ -215,7 +215,7 @@ static const struct versatile_panel_type versatile_panels[] = { .vrefresh = 116, .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, }, - .bus_flags = DRM_BUS_FLAG_PIXDATA_NEGEDGE, + .bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE, .ib2 = true, }, }; diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9322.c b/drivers/gpu/drm/panel/panel-ilitek-ili9322.c index bd38bf4f1ba6..35497ff08391 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9322.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9322.c @@ -412,11 +412,11 @@ static int ili9322_init(struct drm_panel *panel, struct ili9322 *ili) if (ili->conf->dclk_active_high) { reg = ILI9322_POL_DCLK; connector->display_info.bus_flags |= - DRM_BUS_FLAG_PIXDATA_POSEDGE; + DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; } else { reg = 0; connector->display_info.bus_flags |= - DRM_BUS_FLAG_PIXDATA_NEGEDGE; + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE; } if (ili->conf->de_active_high) { reg |= ILI9322_POL_DE; diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c index 75f925390551..198ab13f165d 100644 --- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c +++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c @@ -331,7 +331,7 @@ static const struct seiko_panel_desc seiko_43wvf1g = { .height = 57, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_NEGEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE, }; static const struct of_device_id platform_of_match[] = { diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 97964f7f2ace..49e851d06eb2 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -929,7 +929,7 @@ static const struct panel_desc dataimage_scf0700c48ggu18 = { .height = 91, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, }; static const struct display_timing dlc_dlc0700yzg_1_timing = { @@ -984,7 +984,7 @@ static const struct panel_desc edt_et057090dhu = { .height = 86, }, .bus_format = MEDIA_BUS_FMT_RGB666_1X18, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_NEGEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE, }; static const struct drm_display_mode edt_etm0700g0dh6_mode = { @@ -1010,7 +1010,7 @@ static const struct panel_desc edt_etm0700g0dh6 = { .height = 91, }, .bus_format = MEDIA_BUS_FMT_RGB666_1X18, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_NEGEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE, }; static const struct panel_desc edt_etm0700g0bdh6 = { @@ -1022,7 +1022,7 @@ static const struct panel_desc edt_etm0700g0bdh6 = { .height = 91, }, .bus_format = MEDIA_BUS_FMT_RGB666_1X18, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, }; static const struct drm_display_mode foxlink_fl500wvr00_a0t_mode = { @@ -1176,7 +1176,7 @@ static const struct panel_desc innolux_at043tn24 = { .height = 54, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, }; static const struct drm_display_mode innolux_at070tn92_mode = { @@ -1658,7 +1658,7 @@ static const struct panel_desc nec_nl4827hc19_05b = { .height = 54, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, - .bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE, + .bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, }; static const struct drm_display_mode netron_dy_e231732_mode = { @@ -1707,7 +1707,7 @@ static const struct panel_desc newhaven_nhd_43_480272ef_atxl = { .height = 54, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE | + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE | DRM_BUS_FLAG_SYNC_POSEDGE, }; @@ -1869,7 +1869,7 @@ static const struct panel_desc ortustech_com43h4m85ulc = { .height = 93, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, }; static const struct drm_display_mode qd43003c0_40_mode = { @@ -2214,7 +2214,7 @@ static const struct panel_desc toshiba_lt089ac29000 = { .height = 116, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, - .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_POSEDGE, + .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, }; static const struct drm_display_mode tpk_f07a_0102_mode = { @@ -2237,7 +2237,7 @@ static const struct panel_desc tpk_f07a_0102 = { .width = 152, .height = 91, }, - .bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE, + .bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, }; static const struct drm_display_mode tpk_f10a_0102_mode = { diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c index 754f6b25f265..0c5d391f0a8f 100644 --- a/drivers/gpu/drm/pl111/pl111_display.c +++ b/drivers/gpu/drm/pl111/pl111_display.c @@ -188,7 +188,7 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe, tim2 |= TIM2_IOE; if (connector->display_info.bus_flags & - DRM_BUS_FLAG_PIXDATA_NEGEDGE) + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) tim2 |= TIM2_IPC; } diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index c78cd35a1294..88dea125e76a 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -560,10 +560,10 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon, struct drm_connector *connector = panel->connector; struct drm_display_info display_info = connector->display_info; - if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE) + if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) clk_set_phase(tcon->dclk, 240); - if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) + if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) clk_set_phase(tcon->dclk, 0); } diff --git a/drivers/gpu/drm/tve200/tve200_display.c b/drivers/gpu/drm/tve200/tve200_display.c index e8723a2412a6..d775d10dbe6a 100644 --- a/drivers/gpu/drm/tve200/tve200_display.c +++ b/drivers/gpu/drm/tve200/tve200_display.c @@ -149,7 +149,8 @@ static void tve200_display_enable(struct drm_simple_display_pipe *pipe, /* Vsync IRQ at start of Vsync at first */ ctrl1 |= TVE200_VSTSTYPE_VSYNC; - if (connector->display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) + if (connector->display_info.bus_flags & + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) ctrl1 |= TVE200_CTRL_TVCLKP; if ((mode->hdisplay == 352 && mode->vdisplay == 240) || /* SIF(525) */ diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index bd850747ce54..8d12486181e6 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -246,10 +246,10 @@ struct drm_bridge_timings { /** * @sampling_edge: * - * Tells whether the bridge samples the digital input signal - * from the display engine on the positive or negative edge of the - * clock, this should reuse the DRM_BUS_FLAG_PIXDATA_[POS|NEG]EDGE - * bitwise flags from the DRM connector (bit 2 and 3 valid). + * Tells whether the bridge samples the digital input signals from the + * display engine on the positive or negative edge of the clock. This + * should use the DRM_BUS_FLAG_PIXDATA_SAMPLE_[POS|NEG]EDGE bitwise + * flags from the DRM connector (bit 2 and 3 valid). */ u32 sampling_edge; /** -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags 2018-09-22 12:15 ` [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags Laurent Pinchart @ 2018-09-24 11:48 ` Thierry Reding 2018-09-24 11:59 ` Stefan Agner 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2018-09-24 11:48 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Maxime Ripard, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 4224 bytes --] On Sat, Sep 22, 2018 at 03:15:04PM +0300, Laurent Pinchart wrote: > The DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags are deprecated in favour of > the new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags. Replace > them through the code. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- > drivers/gpu/drm/drm_modes.c | 8 ++++---- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- > drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- > drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- > .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- > .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- > drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- > drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- > drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- > drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- > drivers/gpu/drm/panel/panel-simple.c | 20 ++++++++++---------- > drivers/gpu/drm/pl111/pl111_display.c | 2 +- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > drivers/gpu/drm/tve200/tve200_display.c | 3 ++- > include/drm/drm_bridge.h | 8 ++++---- > 23 files changed, 47 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c > index 9b706789a341..7dc14c22f7db 100644 > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c > @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev) > */ > static const struct drm_bridge_timings default_dac_timings = { > /* Timing specifications, datasheet page 7 */ > - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > .setup_time_ps = 500, > .hold_time_ps = 1500, > }; > @@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = { > */ > static const struct drm_bridge_timings ti_ths8134_dac_timings = { > /* From timing diagram, datasheet page 9 */ > - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > /* From datasheet, page 12 */ > .setup_time_ps = 3000, > /* I guess this means latched input */ > @@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = { > */ > static const struct drm_bridge_timings ti_ths8135_dac_timings = { > /* From timing diagram, datasheet page 14 */ > - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > /* From datasheet, page 16 */ > .setup_time_ps = 2000, > .hold_time_ps = 500, Now I'm confused. Aren't you effectively iverting the sampling edges here? That and the fact that everywhere else we only use the driver variants makes me think that we should just define which way these are supposed to be used and just have a single set of definitions. Also, I think there's no need for these to be always "physically" correct. This is up to interpretation by the driver, so if a bridge driver wants to use them as meaning "sampled edge", that's fine. If display controllers use them to mean "driven edge", that's equally fine. As long as we don't communicate the flags between drivers there should be no reason for them to be confusing. Just make sure that the comments in the interfaces clarify from which point of view these flags are to be interpreted and we should be fine. Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags 2018-09-24 11:48 ` Thierry Reding @ 2018-09-24 11:59 ` Stefan Agner 2018-09-24 12:13 ` Thierry Reding 0 siblings, 1 reply; 11+ messages in thread From: Stefan Agner @ 2018-09-24 11:59 UTC (permalink / raw) To: Thierry Reding; +Cc: Laurent Pinchart, Maxime Ripard, dri-devel On 24.09.2018 13:48, Thierry Reding wrote: > On Sat, Sep 22, 2018 at 03:15:04PM +0300, Laurent Pinchart wrote: >> The DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags are deprecated in favour of >> the new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags. Replace >> them through the code. >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> --- >> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- >> drivers/gpu/drm/drm_modes.c | 8 ++++---- >> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- >> drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- >> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- >> drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- >> .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- >> .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- >> .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- >> .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- >> .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- >> .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- >> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- >> drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- >> drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- >> drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- >> drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- >> drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- >> drivers/gpu/drm/panel/panel-simple.c | 20 ++++++++++---------- >> drivers/gpu/drm/pl111/pl111_display.c | 2 +- >> drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- >> drivers/gpu/drm/tve200/tve200_display.c | 3 ++- >> include/drm/drm_bridge.h | 8 ++++---- >> 23 files changed, 47 insertions(+), 46 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c >> index 9b706789a341..7dc14c22f7db 100644 >> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c >> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c >> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev) >> */ >> static const struct drm_bridge_timings default_dac_timings = { >> /* Timing specifications, datasheet page 7 */ >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, >> .setup_time_ps = 500, >> .hold_time_ps = 1500, >> }; >> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = { >> */ >> static const struct drm_bridge_timings ti_ths8134_dac_timings = { >> /* From timing diagram, datasheet page 9 */ >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, >> /* From datasheet, page 12 */ >> .setup_time_ps = 3000, >> /* I guess this means latched input */ >> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = { >> */ >> static const struct drm_bridge_timings ti_ths8135_dac_timings = { >> /* From timing diagram, datasheet page 14 */ >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, >> /* From datasheet, page 16 */ >> .setup_time_ps = 2000, >> .hold_time_ps = 500, > > Now I'm confused. Aren't you effectively iverting the sampling edges > here? The meaning of the flag has been defined differently for the sampling_edge case, see current comment in include/drm/drm_bridge.h. This is correct. It essentially fixes the flags such that they can be interpreted by the driver with the usual assumption to drive on opposite edge. It is essentially the same as my patch did, but using the new flag: https://patchwork.kernel.org/patch/10589233/ So far, no driver used sampling_edge, so we can change safely at this point. -- Stefan > > That and the fact that everywhere else we only use the driver variants > makes me think that we should just define which way these are supposed > to be used and just have a single set of definitions. > > Also, I think there's no need for these to be always "physically" > correct. This is up to interpretation by the driver, so if a bridge > driver wants to use them as meaning "sampled edge", that's fine. If > display controllers use them to mean "driven edge", that's equally > fine. > > As long as we don't communicate the flags between drivers there should > be no reason for them to be confusing. Just make sure that the comments > in the interfaces clarify from which point of view these flags are to be > interpreted and we should be fine. > > Thierry _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags 2018-09-24 11:59 ` Stefan Agner @ 2018-09-24 12:13 ` Thierry Reding 2018-09-24 12:34 ` Stefan Agner 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2018-09-24 12:13 UTC (permalink / raw) To: Stefan Agner; +Cc: Laurent Pinchart, Maxime Ripard, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 5484 bytes --] On Mon, Sep 24, 2018 at 01:59:25PM +0200, Stefan Agner wrote: > On 24.09.2018 13:48, Thierry Reding wrote: > > On Sat, Sep 22, 2018 at 03:15:04PM +0300, Laurent Pinchart wrote: > >> The DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags are deprecated in favour of > >> the new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags. Replace > >> them through the code. > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >> --- > >> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- > >> drivers/gpu/drm/drm_modes.c | 8 ++++---- > >> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- > >> drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- > >> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- > >> drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- > >> .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- > >> .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- > >> .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- > >> .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- > >> .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- > >> .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- > >> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > >> drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- > >> drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- > >> drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- > >> drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- > >> drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- > >> drivers/gpu/drm/panel/panel-simple.c | 20 ++++++++++---------- > >> drivers/gpu/drm/pl111/pl111_display.c | 2 +- > >> drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > >> drivers/gpu/drm/tve200/tve200_display.c | 3 ++- > >> include/drm/drm_bridge.h | 8 ++++---- > >> 23 files changed, 47 insertions(+), 46 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> index 9b706789a341..7dc14c22f7db 100644 > >> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c > >> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev) > >> */ > >> static const struct drm_bridge_timings default_dac_timings = { > >> /* Timing specifications, datasheet page 7 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >> .setup_time_ps = 500, > >> .hold_time_ps = 1500, > >> }; > >> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = { > >> */ > >> static const struct drm_bridge_timings ti_ths8134_dac_timings = { > >> /* From timing diagram, datasheet page 9 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >> /* From datasheet, page 12 */ > >> .setup_time_ps = 3000, > >> /* I guess this means latched input */ > >> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = { > >> */ > >> static const struct drm_bridge_timings ti_ths8135_dac_timings = { > >> /* From timing diagram, datasheet page 14 */ > >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >> /* From datasheet, page 16 */ > >> .setup_time_ps = 2000, > >> .hold_time_ps = 500, > > > > Now I'm confused. Aren't you effectively iverting the sampling edges > > here? > > The meaning of the flag has been defined differently for the > sampling_edge case, see current comment in include/drm/drm_bridge.h. > > This is correct. It essentially fixes the flags such that they can be > interpreted by the driver with the usual assumption to drive on opposite > edge. It is essentially the same as my patch did, but using the new > flag: > https://patchwork.kernel.org/patch/10589233/ > > So far, no driver used sampling_edge, so we can change safely at this > point. I was just wondering because the above clearly changes the value in .sampling_edge, but if it isn't currently used it doesn't really matter. One potential problem I see here is that the kerneldoc for sampling_edge says that it should reuse the flags from the DRM connector. Now if the DRM connector specifies these from a drive perspective and the bridge interprets them from a sample perspective, this is obviously going to be a problem. But then, the only places where these are used seems to be in the VGA bridge driver, so I'm assuming that these are from a sampling perspective and should be interpreted that way. So I think that's all that we need to define. If a driver specifies the values in some structure, then they should be interpreted from the driver's perspective. If a display driver uses the values provided by a bridge driver it needs to interpret them from a sampling perspective as well. The issue I see with multiple definitions is that it doesn't actually solve the problem. If a bridge driver specifies the flags from a sampling perspective and the display driver interprets them as drive flags, there will still be confusion, right? Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags 2018-09-24 12:13 ` Thierry Reding @ 2018-09-24 12:34 ` Stefan Agner 2018-12-05 8:23 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Stefan Agner @ 2018-09-24 12:34 UTC (permalink / raw) To: Thierry Reding; +Cc: Laurent Pinchart, Maxime Ripard, dri-devel On 24.09.2018 14:13, Thierry Reding wrote: > On Mon, Sep 24, 2018 at 01:59:25PM +0200, Stefan Agner wrote: >> On 24.09.2018 13:48, Thierry Reding wrote: >> > On Sat, Sep 22, 2018 at 03:15:04PM +0300, Laurent Pinchart wrote: >> >> The DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags are deprecated in favour of >> >> the new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags. Replace >> >> them through the code. >> >> >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> >> --- >> >> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- >> >> drivers/gpu/drm/drm_modes.c | 8 ++++---- >> >> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- >> >> drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- >> >> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- >> >> drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- >> >> .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- >> >> .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- >> >> .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- >> >> .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- >> >> .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- >> >> .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- >> >> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- >> >> drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- >> >> drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- >> >> drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- >> >> drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- >> >> drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- >> >> drivers/gpu/drm/panel/panel-simple.c | 20 ++++++++++---------- >> >> drivers/gpu/drm/pl111/pl111_display.c | 2 +- >> >> drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- >> >> drivers/gpu/drm/tve200/tve200_display.c | 3 ++- >> >> include/drm/drm_bridge.h | 8 ++++---- >> >> 23 files changed, 47 insertions(+), 46 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c >> >> index 9b706789a341..7dc14c22f7db 100644 >> >> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c >> >> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c >> >> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device *pdev) >> >> */ >> >> static const struct drm_bridge_timings default_dac_timings = { >> >> /* Timing specifications, datasheet page 7 */ >> >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, >> >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, >> >> .setup_time_ps = 500, >> >> .hold_time_ps = 1500, >> >> }; >> >> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings default_dac_timings = { >> >> */ >> >> static const struct drm_bridge_timings ti_ths8134_dac_timings = { >> >> /* From timing diagram, datasheet page 9 */ >> >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, >> >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, >> >> /* From datasheet, page 12 */ >> >> .setup_time_ps = 3000, >> >> /* I guess this means latched input */ >> >> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings ti_ths8134_dac_timings = { >> >> */ >> >> static const struct drm_bridge_timings ti_ths8135_dac_timings = { >> >> /* From timing diagram, datasheet page 14 */ >> >> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, >> >> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, >> >> /* From datasheet, page 16 */ >> >> .setup_time_ps = 2000, >> >> .hold_time_ps = 500, >> > >> > Now I'm confused. Aren't you effectively iverting the sampling edges >> > here? >> >> The meaning of the flag has been defined differently for the >> sampling_edge case, see current comment in include/drm/drm_bridge.h. >> >> This is correct. It essentially fixes the flags such that they can be >> interpreted by the driver with the usual assumption to drive on opposite >> edge. It is essentially the same as my patch did, but using the new >> flag: >> https://patchwork.kernel.org/patch/10589233/ >> >> So far, no driver used sampling_edge, so we can change safely at this >> point. > > I was just wondering because the above clearly changes the value in > .sampling_edge, but if it isn't currently used it doesn't really matter. > > One potential problem I see here is that the kerneldoc for sampling_edge > says that it should reuse the flags from the DRM connector. Now if the > DRM connector specifies these from a drive perspective and the bridge > interprets them from a sample perspective, this is obviously going to be > a problem. But then, the only places where these are used seems to be in > the VGA bridge driver, so I'm assuming that these are from a sampling > perspective and should be interpreted that way. > > So I think that's all that we need to define. If a driver specifies the > values in some structure, then they should be interpreted from the > driver's perspective. If a display driver uses the values provided by a > bridge driver it needs to interpret them from a sampling perspective as > well. Until struct drm_bridge_timings has been introduced, *everything* was driving perspective. Display timings (DISPLAY_FLAGS_PIXDATA_[POS|NEG]EDGE) and bus flags... struct drm_bridge_timings reuses the bus flags, and with that we have a somewhat conflicting definition: - At the #define site for the flags we document that they are drive perspective - sampling_edge reuses the same flag, but changes its meaning according to kerneldoc This is not ideal, and my patch as well as this patch series is an attempt to clear things up. > > The issue I see with multiple definitions is that it doesn't actually > solve the problem. If a bridge driver specifies the flags from a > sampling perspective and the display driver interprets them as drive > flags, there will still be confusion, right? Having the drive/sample situation encoded in the flag avoids confusion... We assume that by default we drive/sample at the opposite edge, and that is how the flags are defined. What confusion do you still see? My attempt to clear things up was just using the definition we always had: https://lkml.org/lkml/2018/9/12/911 I see Laurents point, but I am not sure if its worth the churn. Especially since we also use driving view for DISPLAY_FLAGS_PIXDATA_[POS|NEG]EDGE (see include/video/display_timing.h). Maybe we should just restate the fact that pixel clock is driving view in kerneldoc of struct drm_bridge_timings... -- Stefan _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags 2018-09-24 12:34 ` Stefan Agner @ 2018-12-05 8:23 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2018-12-05 8:23 UTC (permalink / raw) To: Stefan Agner; +Cc: Laurent Pinchart, Maxime Ripard, dri-devel, Thierry Reding Hello, On Monday, 24 September 2018 15:34:54 EET Stefan Agner wrote: > On 24.09.2018 14:13, Thierry Reding wrote: > > On Mon, Sep 24, 2018 at 01:59:25PM +0200, Stefan Agner wrote: > >> On 24.09.2018 13:48, Thierry Reding wrote: > >>> On Sat, Sep 22, 2018 at 03:15:04PM +0300, Laurent Pinchart wrote: > >>>> The DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags are deprecated in favour > >>>> of the new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags. > >>>> Replace them through the code. > >>>> > >>>> Signed-off-by: Laurent Pinchart > >>>> <laurent.pinchart+renesas@ideasonboard.com> > >>>> --- > >>>> > >>>> drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- > >>>> drivers/gpu/drm/drm_modes.c | 8 ++++---- > >>>> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- > >>>> drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- > >>>> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- > >>>> drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- > >>>> .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- > >>>> .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- > >>>> .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- > >>>> .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- > >>>> .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- > >>>> .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- > >>>> drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > >>>> drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- > >>>> drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- > >>>> drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- > >>>> drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- > >>>> drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- > >>>> drivers/gpu/drm/panel/panel-simple.c | 20 ++++++------ > >>>> drivers/gpu/drm/pl111/pl111_display.c | 2 +- > >>>> drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > >>>> drivers/gpu/drm/tve200/tve200_display.c | 3 ++- > >>>> include/drm/drm_bridge.h | 8 ++++---- > >>>> 23 files changed, 47 insertions(+), 46 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c > >>>> b/drivers/gpu/drm/bridge/dumb-vga-dac.c index > >>>> 9b706789a341..7dc14c22f7db 100644 > >>>> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c > >>>> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c > >>>> @@ -234,7 +234,7 @@ static int dumb_vga_remove(struct platform_device > >>>> *pdev) > >>>> */ > >>>> static const struct drm_bridge_timings default_dac_timings = { > >>>> /* Timing specifications, datasheet page 7 */ > >>>> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >>>> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >>>> .setup_time_ps = 500, > >>>> .hold_time_ps = 1500, > >>>> }; > >>>> @@ -245,7 +245,7 @@ static const struct drm_bridge_timings > >>>> default_dac_timings = { > >>>> */ > >>>> static const struct drm_bridge_timings ti_ths8134_dac_timings = { > >>>> /* From timing diagram, datasheet page 9 */ > >>>> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >>>> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >>>> /* From datasheet, page 12 */ > >>>> .setup_time_ps = 3000, > >>>> /* I guess this means latched input */ > >>>> @@ -258,7 +258,7 @@ static const struct drm_bridge_timings > >>>> ti_ths8134_dac_timings = { > >>>> */ > >>>> static const struct drm_bridge_timings ti_ths8135_dac_timings = { > >>>> /* From timing diagram, datasheet page 14 */ > >>>> - .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE, > >>>> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE, > >>>> /* From datasheet, page 16 */ > >>>> .setup_time_ps = 2000, > >>>> .hold_time_ps = 500, > >>> > >>> Now I'm confused. Aren't you effectively iverting the sampling edges > >>> here? > >> > >> The meaning of the flag has been defined differently for the > >> sampling_edge case, see current comment in include/drm/drm_bridge.h. > >> > >> This is correct. It essentially fixes the flags such that they can be > >> interpreted by the driver with the usual assumption to drive on opposite > >> edge. It is essentially the same as my patch did, but using the new > >> flag: > >> https://patchwork.kernel.org/patch/10589233/ > >> > >> So far, no driver used sampling_edge, so we can change safely at this > >> point. > > > > I was just wondering because the above clearly changes the value in > > .sampling_edge, but if it isn't currently used it doesn't really matter. Your concern was correct, the patch changes the value, and as Stefan explained it is safe as no driver consumes these values. I will however update the commit message to make this explicit. > > One potential problem I see here is that the kerneldoc for sampling_edge > > says that it should reuse the flags from the DRM connector. Now if the > > DRM connector specifies these from a drive perspective and the bridge > > interprets them from a sample perspective, this is obviously going to be > > a problem. But then, the only places where these are used seems to be in > > the VGA bridge driver, so I'm assuming that these are from a sampling > > perspective and should be interpreted that way. > > > > So I think that's all that we need to define. If a driver specifies the > > values in some structure, then they should be interpreted from the > > driver's perspective. If a display driver uses the values provided by a > > bridge driver it needs to interpret them from a sampling perspective as > > well. > > Until struct drm_bridge_timings has been introduced, *everything* was > driving perspective. Display timings > (DISPLAY_FLAGS_PIXDATA_[POS|NEG]EDGE) and bus flags... > > struct drm_bridge_timings reuses the bus flags, and with that we have a > somewhat conflicting definition: > > - At the #define site for the flags we document that they are drive > perspective > - sampling_edge reuses the same flag, but changes its meaning according > to kerneldoc > > This is not ideal, and my patch as well as this patch series is an > attempt to clear things up. > > > The issue I see with multiple definitions is that it doesn't actually > > solve the problem. If a bridge driver specifies the flags from a > > sampling perspective and the display driver interprets them as drive > > flags, there will still be confusion, right? > > Having the drive/sample situation encoded in the flag avoids > confusion... > > We assume that by default we drive/sample at the opposite edge, and that > is how the flags are defined. > > What confusion do you still see? > > > My attempt to clear things up was just using the definition we always > had: > https://lkml.org/lkml/2018/9/12/911 > > I see Laurents point, but I am not sure if its worth the churn. I'll handle the churn :-) > Especially since we also use driving view for > DISPLAY_FLAGS_PIXDATA_[POS|NEG]EDGE (see > include/video/display_timing.h). I will try to have a look at that after handling the DRM side. > Maybe we should just restate the fact that pixel clock is driving view > in kerneldoc of struct drm_bridge_timings... I would prefer avoiding that. I think bridges should be as simple as possible and just expose their own parameters, from their own point of view. The complexity of deciding on which edge to drive should be on the source (display controller) side, based on the sink's (bridge) intrinsic parameters (sampling edge, setup/hold time), on the bus clock and on the source's intrinsic parameters (internal signal delays). The computation should happen in a helper function, and it should then be as simple as the following pseudo-code for display controller drivers. source_timings.pixdata_delay_ps = 8000; driving_edge = drm_timings_driving_edge(bridge, mode, &source_timings); -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags 2018-09-22 12:15 [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Laurent Pinchart 2018-09-22 12:15 ` [RFC/PATCH 1/2] drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros Laurent Pinchart 2018-09-22 12:15 ` [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags Laurent Pinchart @ 2018-09-24 11:54 ` Stefan Agner 2018-12-05 8:08 ` Laurent Pinchart 2018-09-26 7:37 ` Linus Walleij 3 siblings, 1 reply; 11+ messages in thread From: Stefan Agner @ 2018-09-24 11:54 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Maxime Ripard, dri-devel, Thierry Reding On 22.09.2018 14:15, Laurent Pinchart wrote: > Hello, > > This patch series attemps at clarifying usage of the > DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags. It results from a discussion > on the mailing list available at [1]. > > The problem being discussed was confusion around how the > DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE flags > could be interpreted (and are interpreted now by drivers). Patch 1/2 > introduces new, more explicit flags, and explains the rationale. Patch > 2/2 then updates the drivers to use the new flags. IMHO, the meaning was quite clearly stated... I am not sure whether this added clarity is worth the churn. But I am ok with it if others think it's necessary. Btw, if we change this for DRM_BUS_FLAG*, we probably should also do the equal change for DISPLAY_FLAGS_PIXDATA_[NEG|POS]EDGE. Since displays are always on the sample side, it probably has higher chance to get mixed up. -- Stefan > > [1] https://www.spinics.net/lists/arm-kernel/msg677079.html > > Laurent Pinchart (2): > drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros > drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags > > drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- > drivers/gpu/drm/drm_modes.c | 8 ++++---- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- > drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- > drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- > .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- > .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- > .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- > drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- > drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- > drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- > drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- > drivers/gpu/drm/panel/panel-simple.c | 20 ++++++++++---------- > drivers/gpu/drm/pl111/pl111_display.c | 2 +- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > drivers/gpu/drm/tve200/tve200_display.c | 3 ++- > include/drm/drm_bridge.h | 8 ++++---- > include/drm/drm_connector.h | 20 ++++++++++++++++++-- > 24 files changed, 65 insertions(+), 48 deletions(-) _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags 2018-09-24 11:54 ` [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Stefan Agner @ 2018-12-05 8:08 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2018-12-05 8:08 UTC (permalink / raw) To: Stefan Agner; +Cc: Laurent Pinchart, Maxime Ripard, dri-devel, Thierry Reding Hi Stefan, On Monday, 24 September 2018 14:54:36 EET Stefan Agner wrote: > On 22.09.2018 14:15, Laurent Pinchart wrote: > > Hello, > > > > This patch series attemps at clarifying usage of the > > DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags. It results from a discussion > > on the mailing list available at [1]. > > > > The problem being discussed was confusion around how the > > DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE flags > > could be interpreted (and are interpreted now by drivers). Patch 1/2 > > introduces new, more explicit flags, and explains the rationale. Patch > > 2/2 then updates the drivers to use the new flags. > > IMHO, the meaning was quite clearly stated... I am not sure whether this > added clarity is worth the churn. It is stated in a comment, but it's not clear from the macro names, and I had to constantly refer back to the comments to make sure which side the flags applied to when reviewing related patches (or writing related code). This series started as "scratch your own itch" patches, but I think it has more potential than that. By making the drive and sample sides apparent in the API, we let drivers focus on their own side. Of course there's still the implied assumption that sampling on one edge requires driving on the other edge, but I would like to introduce a helper function that computes the driving edge based on all sampling parameters (edge, setup/hold time) and frequency. > But I am ok with it if others think it's necessary. > > Btw, if we change this for DRM_BUS_FLAG*, we probably should also do the > equal change for DISPLAY_FLAGS_PIXDATA_[NEG|POS]EDGE. Since displays are > always on the sample side, it probably has higher chance to get mixed > up. That's a good point. I'd like to focus on the DRM side first to see how that goes, and then address the display timings flags. It would be really nice if we could merge both... > > [1] https://www.spinics.net/lists/arm-kernel/msg677079.html > > > > Laurent Pinchart (2): > > drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros > > drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags > > > > drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +++--- > > drivers/gpu/drm/drm_modes.c | 8 ++++---- > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 2 +- > > drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +- > > drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 6 +++--- > > drivers/gpu/drm/omapdrm/displays/encoder-tfp410.c | 2 +- > > .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c | 2 +- > > .../gpu/drm/omapdrm/displays/panel-nec-nl8048hl11.c | 2 +- > > .../drm/omapdrm/displays/panel-sharp-ls037v7dw01.c | 2 +- > > .../gpu/drm/omapdrm/displays/panel-sony-acx565akm.c | 2 +- > > .../gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c | 2 +- > > .../gpu/drm/omapdrm/displays/panel-tpo-td043mtea1.c | 2 +- > > drivers/gpu/drm/omapdrm/dss/dsi.c | 2 +- > > drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- > > drivers/gpu/drm/omapdrm/omap_encoder.c | 4 ++-- > > drivers/gpu/drm/panel/panel-arm-versatile.c | 4 ++-- > > drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 4 ++-- > > drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 2 +- > > drivers/gpu/drm/panel/panel-simple.c | 20 +++++++-------- > > drivers/gpu/drm/pl111/pl111_display.c | 2 +- > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > > drivers/gpu/drm/tve200/tve200_display.c | 3 ++- > > include/drm/drm_bridge.h | 8 ++++---- > > include/drm/drm_connector.h | 20 +++++++++++++-- > > 24 files changed, 65 insertions(+), 48 deletions(-) -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags 2018-09-22 12:15 [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Laurent Pinchart ` (2 preceding siblings ...) 2018-09-24 11:54 ` [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Stefan Agner @ 2018-09-26 7:37 ` Linus Walleij 3 siblings, 0 replies; 11+ messages in thread From: Linus Walleij @ 2018-09-26 7:37 UTC (permalink / raw) To: Laurent Pinchart Cc: Maxime Ripard, thierry.reding@gmail.com, open list:DRM PANEL DRIVERS On Sat, Sep 22, 2018 at 2:14 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > This patch series attemps at clarifying usage of the > DRM_BUS_FLAG_PIXDATA_(POS|NEG)EDGE flags. It results from a discussion > on the mailing list available at [1]. > > The problem being discussed was confusion around how the > DRM_BUS_FLAG_PIXDATA_POSEDGE and DRM_BUS_FLAG_PIXDATA_NEGEDGE flags > could be interpreted (and are interpreted now by drivers). Patch 1/2 > introduces new, more explicit flags, and explains the rationale. Patch > 2/2 then updates the drivers to use the new flags. > > [1] https://www.spinics.net/lists/arm-kernel/msg677079.html > > Laurent Pinchart (2): > drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros > drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags I am fine with this. Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-12-05 8:23 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-09-22 12:15 [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Laurent Pinchart 2018-09-22 12:15 ` [RFC/PATCH 1/2] drm: Clarify definition of the DRM_BUS_FLAG_PIXDATA_* macros Laurent Pinchart 2018-09-22 12:15 ` [RFC/PATCH 2/2] drm: Use new DRM_BUS_FLAG_PIXDATA_(DRIVE|SAMPLE)_(POS|NEG)EDGE flags Laurent Pinchart 2018-09-24 11:48 ` Thierry Reding 2018-09-24 11:59 ` Stefan Agner 2018-09-24 12:13 ` Thierry Reding 2018-09-24 12:34 ` Stefan Agner 2018-12-05 8:23 ` Laurent Pinchart 2018-09-24 11:54 ` [RFC/PATCH 0/2] Clarify display info PIXDATA bus flags Stefan Agner 2018-12-05 8:08 ` Laurent Pinchart 2018-09-26 7:37 ` Linus Walleij
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.