Hi,
Here's another bunch of cleanups for sun4i-drm. Most of these were found while working on A10/A20 DRM and HDMI support. To be clear, nothing was broken before these patches.
Changes since v1:
- Expanded commit message for patch 5 explaining the details of memory address difference, wrap-around, and why only a few devices were affected.
Patch 1 trims the sun4i-drm probe sequence by not adding repeating components. The component can deal with duplicates, but it leads to excessive debug logs which is confusing for developers not in the know.
Patch 2 moves regmap creation to a point where access is actually possible, i.e. clocks enabled and reset controls deasserted.
Patch 3 moves to the new drm_fb_cma_get_gem_addr() helper instead of open coding the same thing.
Patch 4 expands the comment explaining why we clear the backend registers explicitly.
Patch 5 fixes the buffer address programmed into the backend layer registers. This issue only hits devices with more than 1GB RAM.
Patch 6 documents some newly discovered but unused register bits in the HDMI controller.
Patch 7 delays setting of the PAD_CTRL1 register in the HDMI controller to the mode_set function. The main reason to do this is that between probe time and when the display is actually setup, the controller itself toggles some of the bits in this register. In particular, on the A10 it toggles the invert bits documented in the previous patch. This causes the color to be completed inverted.
Please have a look.
Regards ChenYu
Chen-Yu Tsai (7): drm/sun4i: don't add components that are already in the queue drm/sun4i: backend: Create regmap after access is possible drm/sun4i: backend: Use drm_fb_cma_get_gem_addr() to get display memory drm/sun4i: backend: Add comment explaining why registers are cleared drm/sun4i: backend: Offset layer buffer address by DRAM starting address drm/sun4i: hdmi: Document PAD_CTRL1 output invert bits drm/sun4i: hdmi: Move PAD_CTRL1 setting to mode_set function
drivers/gpu/drm/sun4i/sun4i_backend.c | 45 ++++++++++++++++++---------------- drivers/gpu/drm/sun4i/sun4i_drv.c | 16 ++++++++++++ drivers/gpu/drm/sun4i/sun4i_hdmi.h | 5 ++++ drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 26 ++++++++++++-------- 4 files changed, 61 insertions(+), 31 deletions(-)
The backend has various clocks and reset controls that need to be enabled and deasserted before register access is possible.
Move the creation of the regmap to after the clocks and reset controls have been configured where it makes more sense.
Signed-off-by: Chen-Yu Tsai wens@csie.org --- drivers/gpu/drm/sun4i/sun4i_backend.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index ec5943627aa5..1cc1780f5091 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -369,13 +369,6 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, if (IS_ERR(regs)) return PTR_ERR(regs);
- backend->engine.regs = devm_regmap_init_mmio(dev, regs, - &sun4i_backend_regmap_config); - if (IS_ERR(backend->engine.regs)) { - dev_err(dev, "Couldn't create the backend regmap\n"); - return PTR_ERR(backend->engine.regs); - } - backend->reset = devm_reset_control_get(dev, NULL); if (IS_ERR(backend->reset)) { dev_err(dev, "Couldn't get our reset line\n"); @@ -421,6 +414,13 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, } }
+ backend->engine.regs = devm_regmap_init_mmio(dev, regs, + &sun4i_backend_regmap_config); + if (IS_ERR(backend->engine.regs)) { + dev_err(dev, "Couldn't create the backend regmap\n"); + return PTR_ERR(backend->engine.regs); + } + list_add_tail(&backend->engine.list, &drv->engine_list);
/* Reset the registers */
Even though the components framework can handle duplicate entries, the extra entries cause a lot more debug messages to be generated, which would be confusing to developers not familiar with our driver and the framework in general.
Instead, we can scan the relatively small queue and check if the component to be added is already queued up. Since the display pipelines are symmetrical (not considering the third display pipeline on the A80), and we add components level by level, when we get to the second instance at the same level, any shared downstream components would already be in the queue.
Signed-off-by: Chen-Yu Tsai wens@csie.org --- drivers/gpu/drm/sun4i/sun4i_drv.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index a2012638d5f7..b5879d4620d8 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -226,6 +226,18 @@ struct endpoint_list { struct list_head list; };
+static bool node_is_in_list(struct list_head *endpoints, + struct device_node *node) +{ + struct endpoint_list *endpoint; + + list_for_each_entry(endpoint, endpoints, list) + if (endpoint->node == node) + return true; + + return false; +} + static int sun4i_drv_add_endpoints(struct device *dev, struct list_head *endpoints, struct component_match **match, @@ -292,6 +304,10 @@ static int sun4i_drv_add_endpoints(struct device *dev, } }
+ /* skip downstream node if it is already in the queue */ + if (node_is_in_list(endpoints, remote)) + continue; + /* Add downstream nodes to the queue */ endpoint = kzalloc(sizeof(*endpoint), GFP_KERNEL); if (!endpoint) {
Commit 4636ce93d5b2 ("drm/fb-cma-helper: Add drm_fb_cma_get_gem_addr()") adds a new helper, which covers fetching a drm_framebuffer's GEM object and calculating the buffer address for a given plane.
This patch uses this helper to replace our own open coded version of the same function.
Signed-off-by: Chen-Yu Tsai wens@csie.org --- drivers/gpu/drm/sun4i/sun4i_backend.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 1cc1780f5091..243ddfdc9403 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -209,22 +209,11 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, { struct drm_plane_state *state = plane->state; struct drm_framebuffer *fb = state->fb; - struct drm_gem_cma_object *gem; u32 lo_paddr, hi_paddr; dma_addr_t paddr; - int bpp; - - /* Get the physical address of the buffer in memory */ - gem = drm_fb_cma_get_gem_obj(fb, 0); - - DRM_DEBUG_DRIVER("Using GEM @ %pad\n", &gem->paddr); - - /* Compute the start of the displayed memory */ - bpp = fb->format->cpp[0]; - paddr = gem->paddr + fb->offsets[0]; - paddr += (state->src_x >> 16) * bpp; - paddr += (state->src_y >> 16) * fb->pitches[0];
+ /* Get the start of the displayed memory */ + paddr = drm_fb_cma_get_gem_addr(fb, state, 0); DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
/* Write the 32 lower bits of the address (in bits) */
Many of the backend's layer configuration registers have undefined default values. This poses a risk as we use regmap_update_bits in some places, and don't overwrite the whole register.
At probe/bind time we explicitly clear all the control registers by writing 0 to them. This patch adds a more detailed explanation on why we're doing this.
Signed-off-by: Chen-Yu Tsai wens@csie.org --- drivers/gpu/drm/sun4i/sun4i_backend.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 243ddfdc9403..4fefd8add714 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -412,7 +412,14 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
list_add_tail(&backend->engine.list, &drv->engine_list);
- /* Reset the registers */ + /* + * Many of the backend's layer configuration registers have + * undefined default values. This poses a risk as we use + * regmap_update_bits in some places, and don't overwrite + * the whole register. + * + * Clear the registers here to have something predictable. + */ for (i = 0x800; i < 0x1000; i += 4) regmap_write(backend->engine.regs, i, 0);
The display backend, as well as other peripherals that have a DRAM clock gate and access DRAM directly, bypassing the system bus, address the DRAM starting from 0x0, while physical addresses the system uses starts from 0x40000000 (or 0x20000000 in A80's case).
This issue was witnessed on the Cubietruck, which has 2GB of RAM.
Devices with less RAM function normally due to the DRAM address wrapping around. CMA seems to always allocate its buffer at a very high address, close to the end of DRAM.
On a 1GB RAM device, the physical address would be something like 0x78000000. The DRAM address 0x78000000 would access the same DRAM region as 0x38000000 on a system, as the DRAM address would only span 0x0 ~ 0x3fffffff. The bit 0x40000000 is non-functional in this case.
However on the Cubietruck, the DRAM is 2GB. The physical address is 0x40000000 ~ 0xbfffffff. The buffer would be something like 0xb8000000. But the DRAM address span 0x0 ~ 0x7fffffff, meaning the buffer address wraps around to 0x38000000, which is wrong. The correct DRAM address for it should be 0x78000000.
Correct the address configured into the backend layer registers by PHYS_OFFSET to account for this.
Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") Signed-off-by: Chen-Yu Tsai wens@csie.org --- drivers/gpu/drm/sun4i/sun4i_backend.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 4fefd8add714..d18d7e88d511 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -216,6 +216,13 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, paddr = drm_fb_cma_get_gem_addr(fb, state, 0); DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
+ /* + * backend DMA accesses DRAM directly, bypassing the system + * bus. As such, the address range is different and the buffer + * address needs to be corrected. + */ + paddr -= PHYS_OFFSET; + /* Write the 32 lower bits of the address (in bits) */ lo_paddr = paddr << 3; DRM_DEBUG_DRIVER("Setting address lower bits to 0x%x\n", lo_paddr);
On Tue, Oct 17, 2017 at 12:23:47PM +0800, Chen-Yu Tsai wrote:
The display backend, as well as other peripherals that have a DRAM clock gate and access DRAM directly, bypassing the system bus, address the DRAM starting from 0x0, while physical addresses the system uses starts from 0x40000000 (or 0x20000000 in A80's case).
This issue was witnessed on the Cubietruck, which has 2GB of RAM.
Devices with less RAM function normally due to the DRAM address wrapping around. CMA seems to always allocate its buffer at a very high address, close to the end of DRAM.
On a 1GB RAM device, the physical address would be something like 0x78000000. The DRAM address 0x78000000 would access the same DRAM region as 0x38000000 on a system, as the DRAM address would only span 0x0 ~ 0x3fffffff. The bit 0x40000000 is non-functional in this case.
However on the Cubietruck, the DRAM is 2GB. The physical address is 0x40000000 ~ 0xbfffffff. The buffer would be something like 0xb8000000. But the DRAM address span 0x0 ~ 0x7fffffff, meaning the buffer address wraps around to 0x38000000, which is wrong. The correct DRAM address for it should be 0x78000000.
Correct the address configured into the backend layer registers by PHYS_OFFSET to account for this.
Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") Signed-off-by: Chen-Yu Tsai wens@csie.org
Applied, thanks! Maxime
While debugging inverted color from the HDMI output on the A10, I found that the lowest 3 bits were set. These were cleared on A20 boards that had normal display output. By manually toggling these bits the mapping of the color components to these bits was found.
While these are not used anywhere, it would be nice to document them somewhere.
Signed-off-by: Chen-Yu Tsai wens@csie.org --- drivers/gpu/drm/sun4i/sun4i_hdmi.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 9b97da39927e..b685ee11623d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -72,6 +72,11 @@ #define SUN4I_HDMI_PAD_CTRL1_HALVE_CLK BIT(6) #define SUN4I_HDMI_PAD_CTRL1_REG_AMP(n) (((n) & 7) << 3)
+/* These bits seem to invert the TMDS data channels */ +#define SUN4I_HDMI_PAD_CTRL1_INVERT_R BIT(2) +#define SUN4I_HDMI_PAD_CTRL1_INVERT_G BIT(1) +#define SUN4I_HDMI_PAD_CTRL1_INVERT_B BIT(0) + #define SUN4I_HDMI_PLL_CTRL_REG 0x208 #define SUN4I_HDMI_PLL_CTRL_PLL_EN BIT(31) #define SUN4I_HDMI_PLL_CTRL_BWS BIT(30)
Initially we configured the PAD_CTRL1 register at probe/bind time. However it seems the HDMI controller will modify some of the bits in this register by itself. On the A10 it is particularly annoying as it toggles the output invert bits, which inverts the colors on the display output.
The U-boot driver this driver is based on sets this register twice, though it seems it's only needed for actual display output. Hence we move it to the mode_set function.
Signed-off-by: Chen-Yu Tsai wens@csie.org --- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index 027b5608dbe6..6ca6e6a74c4a 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -144,6 +144,22 @@ static void sun4i_hdmi_mode_set(struct drm_encoder *encoder, writel(SUN4I_HDMI_UNKNOWN_INPUT_SYNC, hdmi->base + SUN4I_HDMI_UNKNOWN_REG);
+ /* + * Setup output pad (?) controls + * + * This is done here instead of at probe/bind time because + * the controller seems to toggle some of the bits on its own. + * + * We can't just initialize the register there, we need to + * protect the clock bits that have already been read out and + * cached by the clock framework. + */ + val = readl(hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG); + val &= SUN4I_HDMI_PAD_CTRL1_HALVE_CLK; + val |= hdmi->variant->pad_ctrl1_init_val; + writel(val, hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG); + val = readl(hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG); + /* Setup timing registers */ writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) | SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay), @@ -489,16 +505,6 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master, writel(hdmi->variant->pad_ctrl0_init_val, hdmi->base + SUN4I_HDMI_PAD_CTRL0_REG);
- /* - * We can't just initialize the register there, we need to - * protect the clock bits that have already been read out and - * cached by the clock framework. - */ - reg = readl(hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG); - reg &= SUN4I_HDMI_PAD_CTRL1_HALVE_CLK; - reg |= hdmi->variant->pad_ctrl1_init_val; - writel(reg, hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG); - reg = readl(hdmi->base + SUN4I_HDMI_PLL_CTRL_REG); reg &= SUN4I_HDMI_PLL_CTRL_DIV_MASK; reg |= hdmi->variant->pll_ctrl_init_val;
On Tue, Oct 17, 2017 at 12:23 PM, Chen-Yu Tsai wens@csie.org wrote:
Hi,
Here's another bunch of cleanups for sun4i-drm. Most of these were found while working on A10/A20 DRM and HDMI support. To be clear, nothing was broken before these patches.
Changes since v1:
- Expanded commit message for patch 5 explaining the details of memory address difference, wrap-around, and why only a few devices were affected.
Sorry. The subject should have said "v2".
ChenYu
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Maxime Ripard