Hi,
This patchset rework the ingenic-drm driver, improving the code in various places.
The most important change is the last patch, which updates the ingenic-drm driver to use a top-level bridge per output, making use of the bus format and flag negociation implemented in the bridge code. All the external bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Cheers, -Paul
Paul Cercueil (8): drm/ingenic: Remove dead code drm/ingenic: Simplify code by using hwdescs array drm/ingenic: Use standard drm_atomic_helper_commit_tail drm/ingenic: Add support for private objects drm/ingenic: Move IPU scale settings to private state drm/ingenic: Set DMA descriptor chain register when starting CRTC drm/ingenic: Upload palette before frame drm/ingenic: Attach bridge chain to encoders
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 287 ++++++++++++++++------ drivers/gpu/drm/ingenic/ingenic-ipu.c | 127 ++++++++-- 2 files changed, 322 insertions(+), 92 deletions(-)
The priv->ipu_plane would get a different value further down the code, without the first assigned value being read first; so the first assignation can be dropped.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index d261f7a03b18..e42eb43d8020 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL | (sizeof(priv->dma_hwdescs->palette) / 4);
- if (soc_info->has_osd) - priv->ipu_plane = drm_plane_from_index(drm, 0); - primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
Am 08.08.21 um 15:45 schrieb Paul Cercueil:
The priv->ipu_plane would get a different value further down the code, without the first assigned value being read first; so the first assignation can be dropped.
Signed-off-by: Paul Cercueil paul@crapouillou.net
Acked-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index d261f7a03b18..e42eb43d8020 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL | (sizeof(priv->dma_hwdescs->palette) / 4);
if (soc_info->has_osd)
priv->ipu_plane = drm_plane_from_index(drm, 0);primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
On Sun, 2021-08-08 at 19:58 +0200, Thomas Zimmermann wrote:
Am 08.08.21 um 15:45 schrieb Paul Cercueil:
The priv->ipu_plane would get a different value further down the code, without the first assigned value being read first; so the first assignation can be dropped.
Signed-off-by: Paul Cercueil paul@crapouillou.net
Acked-by: Thomas Zimmermann tzimmermann@suse.de
I think this is at best an incomplete description.
How is it known that this priv->ipu_plane assignment isn't necessary for any path of any failure path after this assignment and before the new assignment?
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[]
@@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL | (sizeof(priv->dma_hwdescs->palette) / 4);
- if (soc_info->has_osd)
priv->ipu_plane = drm_plane_from_index(drm, 0);primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
Hi Joe,
Le dim., août 8 2021 at 11:27:34 -0700, Joe Perches joe@perches.com a écrit :
On Sun, 2021-08-08 at 19:58 +0200, Thomas Zimmermann wrote:
Am 08.08.21 um 15:45 schrieb Paul Cercueil:
The priv->ipu_plane would get a different value further down the
code,
without the first assigned value being read first; so the first assignation can be dropped.
Signed-off-by: Paul Cercueil paul@crapouillou.net
Acked-by: Thomas Zimmermann tzimmermann@suse.de
I think this is at best an incomplete description.
How is it known that this priv->ipu_plane assignment isn't necessary for any path of any failure path after this assignment and before the new assignment?
It is only used in the .atomic_begin and .atomic_check callbacks of the CRTC. These will only ever be called after the call to drm_dev_register() which happens at the end of the probe function.
Cheers, -Paul
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[]
@@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device
*dev, bool has_components)
priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL | (sizeof(priv->dma_hwdescs->palette) / 4);
if (soc_info->has_osd)
priv->ipu_plane = drm_plane_from_index(drm, 0);primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
Instead of having one 'hwdesc' variable for the plane #0 and one for the plane #1, use a 'hwdesc[2]' array, where the DMA hardware descriptors are indexed by the plane's number.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38 ++++++++++++----------- 1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index e42eb43d8020..bc71ba44ccf4 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -49,8 +49,7 @@ struct ingenic_dma_hwdesc { } __aligned(16);
struct ingenic_dma_hwdescs { - struct ingenic_dma_hwdesc hwdesc_f0; - struct ingenic_dma_hwdesc hwdesc_f1; + struct ingenic_dma_hwdesc hwdesc[2]; struct ingenic_dma_hwdesc hwdesc_pal; u16 palette[256] __aligned(16); }; @@ -141,6 +140,13 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb) return container_of(nb, struct ingenic_drm, clock_nb); }
+static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv, bool use_f1) +{ + u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[use_f1]); + + return priv->dma_hwdescs_phys + offset; +} + static int ingenic_drm_update_pixclk(struct notifier_block *nb, unsigned long action, void *data) @@ -562,6 +568,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, struct ingenic_dma_hwdesc *hwdesc; unsigned int width, height, cpp, offset; dma_addr_t addr; + bool use_f1; u32 fourcc;
if (newstate && newstate->fb) { @@ -569,16 +576,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);
crtc_state = newstate->crtc->state; + use_f1 = priv->soc_info->has_osd && plane != &priv->f0;
addr = drm_fb_cma_get_gem_addr(newstate->fb, newstate, 0); width = newstate->src_w >> 16; height = newstate->src_h >> 16; cpp = newstate->fb->format->cpp[0];
- if (!priv->soc_info->has_osd || plane == &priv->f0) - hwdesc = &priv->dma_hwdescs->hwdesc_f0; - else - hwdesc = &priv->dma_hwdescs->hwdesc_f1; + hwdesc = &priv->dma_hwdescs->hwdesc[use_f1];
hwdesc->addr = addr; hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4); @@ -591,9 +596,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, if (fourcc == DRM_FORMAT_C8) offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal); else - offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0); + offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);
- priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset; + priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset;
crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8; } @@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
/* Configure DMA hwdesc for foreground0 plane */ - dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys - + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0); - priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0; - priv->dma_hwdescs->hwdesc_f0.id = 0xf0; + dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0); + priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0; + priv->dma_hwdescs->hwdesc[0].id = 0xf0;
/* Configure DMA hwdesc for foreground1 plane */ - dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys - + offsetof(struct ingenic_dma_hwdescs, hwdesc_f1); - priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1; - priv->dma_hwdescs->hwdesc_f1.id = 0xf1; + dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1); + priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1; + priv->dma_hwdescs->hwdesc[1].id = 0xf1;
/* Configure DMA hwdesc for palette */ - priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys - + offsetof(struct ingenic_dma_hwdescs, hwdesc_f0); + priv->dma_hwdescs->hwdesc_pal.next = dma_hwdesc_phys_f0; priv->dma_hwdescs->hwdesc_pal.id = 0xc0; priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys + offsetof(struct ingenic_dma_hwdescs, palette);
Hi
Am 08.08.21 um 15:45 schrieb Paul Cercueil:
Instead of having one 'hwdesc' variable for the plane #0 and one for the plane #1, use a 'hwdesc[2]' array, where the DMA hardware descriptors are indexed by the plane's number.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38 ++++++++++++----------- 1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index e42eb43d8020..bc71ba44ccf4 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -49,8 +49,7 @@ struct ingenic_dma_hwdesc { } __aligned(16);
struct ingenic_dma_hwdescs {
- struct ingenic_dma_hwdesc hwdesc_f0;
- struct ingenic_dma_hwdesc hwdesc_f1;
- struct ingenic_dma_hwdesc hwdesc[2]; struct ingenic_dma_hwdesc hwdesc_pal; u16 palette[256] __aligned(16); };
@@ -141,6 +140,13 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb) return container_of(nb, struct ingenic_drm, clock_nb); }
+static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv, bool use_f1)
Using the plane index instead of a boolean would be more aligned to the way this function is being used.
+{
- u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[use_f1]);
use_f1 is a function parameter. Is offsetof guaranteed to be evaluated at runtime?
- return priv->dma_hwdescs_phys + offset;
+}
- static int ingenic_drm_update_pixclk(struct notifier_block *nb, unsigned long action, void *data)
@@ -562,6 +568,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, struct ingenic_dma_hwdesc *hwdesc; unsigned int width, height, cpp, offset; dma_addr_t addr;
bool use_f1; u32 fourcc;
if (newstate && newstate->fb) {
@@ -569,16 +576,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate);
crtc_state = newstate->crtc->state;
use_f1 = priv->soc_info->has_osd && plane != &priv->f0;addr = drm_fb_cma_get_gem_addr(newstate->fb, newstate, 0); width = newstate->src_w >> 16; height = newstate->src_h >> 16; cpp = newstate->fb->format->cpp[0];
if (!priv->soc_info->has_osd || plane == &priv->f0)hwdesc = &priv->dma_hwdescs->hwdesc_f0;elsehwdesc = &priv->dma_hwdescs->hwdesc_f1;
hwdesc = &priv->dma_hwdescs->hwdesc[use_f1];
Maybe add a helper that looks up the hwdesc field for a given plane?
hwdesc->addr = addr; hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);@@ -591,9 +596,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, if (fourcc == DRM_FORMAT_C8) offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal); else
offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);
priv->dma_hwdescs->hwdesc_f0.next = priv->dma_hwdescs_phys + offset;
priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset;
Maybe priv->dma_hwdescs_phys + offset could be computed in a more flexible helper than dma_hwdesc_addr().
crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8; }@@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
/* Configure DMA hwdesc for foreground0 plane */
- dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);- priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0;
- priv->dma_hwdescs->hwdesc_f0.id = 0xf0;
dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0);
priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0;
priv->dma_hwdescs->hwdesc[0].id = 0xf0;
/* Configure DMA hwdesc for foreground1 plane */
- dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f1);- priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1;
- priv->dma_hwdescs->hwdesc_f1.id = 0xf1;
dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1);
priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1;
priv->dma_hwdescs->hwdesc[1].id = 0xf1;
/* Configure DMA hwdesc for palette */
- priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
- priv->dma_hwdescs->hwdesc_pal.next = dma_hwdesc_phys_f0; priv->dma_hwdescs->hwdesc_pal.id = 0xc0; priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
- offsetof(struct ingenic_dma_hwdescs, palette);
Could the setup in these three blocks be moved into a common helper?
Best regards Thomas
Hi Thomas,
Le dim., août 8 2021 at 20:42:53 +0200, Thomas Zimmermann tzimmermann@suse.de a écrit :
Hi
Am 08.08.21 um 15:45 schrieb Paul Cercueil:
Instead of having one 'hwdesc' variable for the plane #0 and one for the plane #1, use a 'hwdesc[2]' array, where the DMA hardware descriptors are indexed by the plane's number.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 38 ++++++++++++----------- 1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index e42eb43d8020..bc71ba44ccf4 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -49,8 +49,7 @@ struct ingenic_dma_hwdesc { } __aligned(16); struct ingenic_dma_hwdescs {
- struct ingenic_dma_hwdesc hwdesc_f0;
- struct ingenic_dma_hwdesc hwdesc_f1;
- struct ingenic_dma_hwdesc hwdesc[2]; struct ingenic_dma_hwdesc hwdesc_pal; u16 palette[256] __aligned(16); };
@@ -141,6 +140,13 @@ static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb) return container_of(nb, struct ingenic_drm, clock_nb); } +static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv, bool use_f1)
Using the plane index instead of a boolean would be more aligned to the way this function is being used.
Alright, I can do that.
+{
- u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[use_f1]);
use_f1 is a function parameter. Is offsetof guaranteed to be evaluated at runtime?
The offsetof() macro could be defined like this: #define offsetof(type, elm) ((size_t) &((type *) 0)->elm)
So I don't see a reason why this couldn't be evaluated at runtime, yes. It's just that the value of "offset" is not known at compilation time (unless the compiler does some constant propagation). In practice though, this code works fine.
- return priv->dma_hwdescs_phys + offset;
+}
- static int ingenic_drm_update_pixclk(struct notifier_block *nb, unsigned long action, void *data)
@@ -562,6 +568,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, struct ingenic_dma_hwdesc *hwdesc; unsigned int width, height, cpp, offset; dma_addr_t addr;
- bool use_f1; u32 fourcc; if (newstate && newstate->fb) {
@@ -569,16 +576,14 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, drm_fb_cma_sync_non_coherent(&priv->drm, oldstate, newstate); crtc_state = newstate->crtc->state;
use_f1 = priv->soc_info->has_osd && plane != &priv->f0;
hwdesc = &priv->dma_hwdescs->hwdesc_f0;elsehwdesc = &priv->dma_hwdescs->hwdesc_f1;
hwdesc = &priv->dma_hwdescs->hwdesc[use_f1];Maybe add a helper that looks up the hwdesc field for a given plane?
Sure.
hwdesc->addr = addr; hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4); @@ -591,9 +596,9 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, if (fourcc == DRM_FORMAT_C8) offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal); else
offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]);offset;
priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys +offset;
Maybe priv->dma_hwdescs_phys + offset could be computed in a more flexible helper than dma_hwdesc_addr().
crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8; } @@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) /* Configure DMA hwdesc for foreground0 plane */
- dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);- priv->dma_hwdescs->hwdesc_f0.next = dma_hwdesc_phys_f0;
- priv->dma_hwdescs->hwdesc_f0.id = 0xf0;
- dma_hwdesc_phys_f0 = dma_hwdesc_addr(priv, 0);
- priv->dma_hwdescs->hwdesc[0].next = dma_hwdesc_phys_f0;
- priv->dma_hwdescs->hwdesc[0].id = 0xf0; /* Configure DMA hwdesc for foreground1 plane */
- dma_hwdesc_phys_f1 = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f1);- priv->dma_hwdescs->hwdesc_f1.next = dma_hwdesc_phys_f1;
- priv->dma_hwdescs->hwdesc_f1.id = 0xf1;
- dma_hwdesc_phys_f1 = dma_hwdesc_addr(priv, 1);
- priv->dma_hwdescs->hwdesc[1].next = dma_hwdesc_phys_f1;
- priv->dma_hwdescs->hwdesc[1].id = 0xf1; /* Configure DMA hwdesc for palette */
- priv->dma_hwdescs->hwdesc_pal.next = priv->dma_hwdescs_phys
+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
- priv->dma_hwdescs->hwdesc_pal.next = dma_hwdesc_phys_f0; priv->dma_hwdescs->hwdesc_pal.id = 0xc0; priv->dma_hwdescs->hwdesc_pal.addr = priv->dma_hwdescs_phys
- offsetof(struct ingenic_dma_hwdescs, palette);
Could the setup in these three blocks be moved into a common helper?
Yes.
Thanks for the review.
Cheers, -Paul
By making the CRTC's .vblank_enable() function return an error when it is known that the hardware won't deliver a VBLANK, we can drop the ingenic_drm_atomic_helper_commit_tail() function and use the standard drm_atomic_helper_commit_tail() function instead.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++------------------- 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index bc71ba44ccf4..3ed7c27a8dde 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, } }
-static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) -{ - /* - * Just your regular drm_atomic_helper_commit_tail(), but only calls - * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank. - */ - struct drm_device *dev = old_state->dev; - struct ingenic_drm *priv = drm_device_get_priv(dev); - - drm_atomic_helper_commit_modeset_disables(dev, old_state); - - drm_atomic_helper_commit_planes(dev, old_state, 0); - - drm_atomic_helper_commit_modeset_enables(dev, old_state); - - drm_atomic_helper_commit_hw_done(old_state); - - if (!priv->no_vblank) - drm_atomic_helper_wait_for_vblanks(dev, old_state); - - drm_atomic_helper_cleanup_planes(dev, old_state); -} - static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg) { struct ingenic_drm *priv = drm_device_get_priv(arg); @@ -749,6 +726,9 @@ static int ingenic_drm_enable_vblank(struct drm_crtc *crtc) { struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+ if (priv->no_vblank) + return -EWOULDBLOCK; + regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
@@ -856,7 +836,7 @@ static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { };
static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = { - .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail, + .atomic_commit_tail = drm_atomic_helper_commit_tail, };
static void ingenic_drm_unbind_all(void *d)
Le 08/08/2021 à 15:45, Paul Cercueil a écrit :
By making the CRTC's .vblank_enable() function return an error when it is known that the hardware won't deliver a VBLANK, we can drop the ingenic_drm_atomic_helper_commit_tail() function and use the standard drm_atomic_helper_commit_tail() function instead.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++------------------- 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index bc71ba44ccf4..3ed7c27a8dde 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, } }
-static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) -{
- /*
* Just your regular drm_atomic_helper_commit_tail(), but only calls* drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.*/- struct drm_device *dev = old_state->dev;
- struct ingenic_drm *priv = drm_device_get_priv(dev);
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
- drm_atomic_helper_commit_planes(dev, old_state, 0);
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
- drm_atomic_helper_commit_hw_done(old_state);
- if (!priv->no_vblank)
drm_atomic_helper_wait_for_vblanks(dev, old_state);- drm_atomic_helper_cleanup_planes(dev, old_state);
-}
Hi, if this function is removed, shouldn't: static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = { .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail, }; be updated as well?
I've not seen it in the serie.
Just my 2v. CJ
Hi Christophe,
Le dim., août 8 2021 at 21:50:04 +0200, Christophe JAILLET christophe.jaillet@wanadoo.fr a écrit :
Le 08/08/2021 à 15:45, Paul Cercueil a écrit :
By making the CRTC's .vblank_enable() function return an error when it is known that the hardware won't deliver a VBLANK, we can drop the ingenic_drm_atomic_helper_commit_tail() function and use the standard drm_atomic_helper_commit_tail() function instead.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++------------------- 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index bc71ba44ccf4..3ed7c27a8dde 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, } } -static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) -{
- /*
* Just your regular drm_atomic_helper_commit_tail(), but onlycalls
* drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.*/- struct drm_device *dev = old_state->dev;
- struct ingenic_drm *priv = drm_device_get_priv(dev);
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
- drm_atomic_helper_commit_planes(dev, old_state, 0);
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
- drm_atomic_helper_commit_hw_done(old_state);
- if (!priv->no_vblank)
drm_atomic_helper_wait_for_vblanks(dev, old_state);- drm_atomic_helper_cleanup_planes(dev, old_state);
-}
Hi, if this function is removed, shouldn't: static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = { .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail, }; be updated as well?
I've not seen it in the serie.
It is there though :) At the bottom of this very patch.
Just my 2v. CJ
Cheers, -Paul
Le 08/08/2021 à 22:09, Paul Cercueil a écrit :
Hi Christophe,
Le dim., août 8 2021 at 21:50:04 +0200, Christophe JAILLET christophe.jaillet@wanadoo.fr a écrit :
Le 08/08/2021 à 15:45, Paul Cercueil a écrit :
By making the CRTC's .vblank_enable() function return an error when it is known that the hardware won't deliver a VBLANK, we can drop the ingenic_drm_atomic_helper_commit_tail() function and use the standard drm_atomic_helper_commit_tail() function instead.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++------------------- 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index bc71ba44ccf4..3ed7c27a8dde 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, } } -static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) -{ - /* - * Just your regular drm_atomic_helper_commit_tail(), but only calls - * drm_atomic_helper_wait_for_vblanks() if priv->no_vblank. - */ - struct drm_device *dev = old_state->dev; - struct ingenic_drm *priv = drm_device_get_priv(dev);
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
- drm_atomic_helper_commit_planes(dev, old_state, 0);
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
- drm_atomic_helper_commit_hw_done(old_state);
- if (!priv->no_vblank) - drm_atomic_helper_wait_for_vblanks(dev, old_state);
- drm_atomic_helper_cleanup_planes(dev, old_state); -}
Hi, if this function is removed, shouldn't: static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = { .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail, }; be updated as well?
I've not seen it in the serie.
It is there though :) At the bottom of this very patch.
My email client played me some tricks, apparently! Sorry for the noise.
CJ
Just my 2v. CJ
Cheers, -Paul
On Sun, Aug 08, 2021 at 03:45:21PM +0200, Paul Cercueil wrote:
By making the CRTC's .vblank_enable() function return an error when it is known that the hardware won't deliver a VBLANK, we can drop the ingenic_drm_atomic_helper_commit_tail() function and use the standard drm_atomic_helper_commit_tail() function instead.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++------------------- 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index bc71ba44ccf4..3ed7c27a8dde 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, } }
-static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) -{
- /*
* Just your regular drm_atomic_helper_commit_tail(), but only calls* drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.*/- struct drm_device *dev = old_state->dev;
- struct ingenic_drm *priv = drm_device_get_priv(dev);
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
- drm_atomic_helper_commit_planes(dev, old_state, 0);
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
- drm_atomic_helper_commit_hw_done(old_state);
- if (!priv->no_vblank)
drm_atomic_helper_wait_for_vblanks(dev, old_state);- drm_atomic_helper_cleanup_planes(dev, old_state);
-}
static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg) { struct ingenic_drm *priv = drm_device_get_priv(arg); @@ -749,6 +726,9 @@ static int ingenic_drm_enable_vblank(struct drm_crtc *crtc) { struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
- if (priv->no_vblank)
return -EWOULDBLOCK;
I think other drivers return EINVAL here. I'm not sure exactly this is specified, but the errno here is visible to userspace.
Maybe would be good to update the kerneldoc for this hook?
Anyway this is great, with the errno fixed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
- regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
@@ -856,7 +836,7 @@ static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { };
static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = {
- .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
- .atomic_commit_tail = drm_atomic_helper_commit_tail,
};
static void ingenic_drm_unbind_all(void *d)
2.30.2
Hi Daniel,
Le mar., août 10 2021 at 12:20:38 +0200, Daniel Vetter daniel@ffwll.ch a écrit :
On Sun, Aug 08, 2021 at 03:45:21PM +0200, Paul Cercueil wrote:
By making the CRTC's .vblank_enable() function return an error when it is known that the hardware won't deliver a VBLANK, we can drop the ingenic_drm_atomic_helper_commit_tail() function and use the standard drm_atomic_helper_commit_tail() function instead.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 28 ++++------------------- 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index bc71ba44ccf4..3ed7c27a8dde 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -706,29 +706,6 @@ static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, } }
-static void ingenic_drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) -{
- /*
* Just your regular drm_atomic_helper_commit_tail(), but onlycalls
* drm_atomic_helper_wait_for_vblanks() if priv->no_vblank.*/- struct drm_device *dev = old_state->dev;
- struct ingenic_drm *priv = drm_device_get_priv(dev);
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
- drm_atomic_helper_commit_planes(dev, old_state, 0);
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
- drm_atomic_helper_commit_hw_done(old_state);
- if (!priv->no_vblank)
drm_atomic_helper_wait_for_vblanks(dev, old_state);- drm_atomic_helper_cleanup_planes(dev, old_state);
-}
static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg) { struct ingenic_drm *priv = drm_device_get_priv(arg); @@ -749,6 +726,9 @@ static int ingenic_drm_enable_vblank(struct drm_crtc *crtc) { struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
- if (priv->no_vblank)
return -EWOULDBLOCK;I think other drivers return EINVAL here. I'm not sure exactly this is specified, but the errno here is visible to userspace.
Maybe would be good to update the kerneldoc for this hook?
Anyway this is great, with the errno fixed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
I thought about it afterwards, that my error code wasn't great. In my mind it was "the driver will hang while waiting for vblank" hence -EWOULDBLOCK ;)
I'll need to v2 anyway so I'll change it to -EINVAL then.
Cheers, -Paul
- regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
@@ -856,7 +836,7 @@ static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { };
static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = {
- .atomic_commit_tail = ingenic_drm_atomic_helper_commit_tail,
- .atomic_commit_tail = drm_atomic_helper_commit_tail,
};
static void ingenic_drm_unbind_all(void *d)
2.30.2
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Until now, the ingenic-drm as well as the ingenic-ipu drivers used to put state-specific information in their respective private structure.
Add boilerplate code to support private objects in the two drivers, so that state-specific information can be put in the state-specific private structure.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 61 +++++++++++++++++++++++ drivers/gpu/drm/ingenic/ingenic-ipu.c | 54 ++++++++++++++++++++ 2 files changed, 115 insertions(+)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 3ed7c27a8dde..3fc01cffb00f 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -63,6 +63,10 @@ struct jz_soc_info { unsigned int num_formats_f0, num_formats_f1; };
+struct ingenic_drm_private_state { + struct drm_private_state base; +}; + struct ingenic_drm { struct drm_device drm; /* @@ -98,8 +102,16 @@ struct ingenic_drm { struct mutex clk_mutex; bool update_clk_rate; struct notifier_block clock_nb; + + struct drm_private_obj private_obj; };
+static inline struct ingenic_drm_private_state * +to_ingenic_drm_priv_state(struct drm_private_state *state) +{ + return container_of(state, struct ingenic_drm_private_state, base); +} + static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg) { switch (reg) { @@ -769,6 +781,28 @@ ingenic_drm_gem_create_object(struct drm_device *drm, size_t size) return &obj->base; }
+static struct drm_private_state * +ingenic_drm_duplicate_state(struct drm_private_obj *obj) +{ + struct ingenic_drm_private_state *state = to_ingenic_drm_priv_state(obj->state); + + state = kmemdup(state, sizeof(*state), GFP_KERNEL); + if (!state) + return NULL; + + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base); + + return &state->base; +} + +static void ingenic_drm_destroy_state(struct drm_private_obj *obj, + struct drm_private_state *state) +{ + struct ingenic_drm_private_state *priv_state = to_ingenic_drm_priv_state(state); + + kfree(priv_state); +} + DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
static const struct drm_driver ingenic_drm_driver_data = { @@ -839,6 +873,11 @@ static struct drm_mode_config_helper_funcs ingenic_drm_mode_config_helpers = { .atomic_commit_tail = drm_atomic_helper_commit_tail, };
+static const struct drm_private_state_funcs ingenic_drm_private_state_funcs = { + .atomic_duplicate_state = ingenic_drm_duplicate_state, + .atomic_destroy_state = ingenic_drm_destroy_state, +}; + static void ingenic_drm_unbind_all(void *d) { struct ingenic_drm *priv = d; @@ -851,9 +890,15 @@ static void __maybe_unused ingenic_drm_release_rmem(void *d) of_reserved_mem_device_release(d); }
+static void ingenic_drm_atomic_private_obj_fini(struct drm_device *drm, void *private_obj) +{ + drm_atomic_private_obj_fini(private_obj); +} + static int ingenic_drm_bind(struct device *dev, bool has_components) { struct platform_device *pdev = to_platform_device(dev); + struct ingenic_drm_private_state *private_state; const struct jz_soc_info *soc_info; struct ingenic_drm *priv; struct clk *parent_clk; @@ -1132,6 +1177,20 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) goto err_devclk_disable; }
+ private_state = kzalloc(sizeof(*private_state), GFP_KERNEL); + if (!private_state) { + ret = -ENOMEM; + goto err_clk_notifier_unregister; + } + + drm_atomic_private_obj_init(drm, &priv->private_obj, &private_state->base, + &ingenic_drm_private_state_funcs); + + ret = drmm_add_action_or_reset(drm, ingenic_drm_atomic_private_obj_fini, + &priv->private_obj); + if (ret) + goto err_private_state_free; + ret = drm_dev_register(drm, 0); if (ret) { dev_err(dev, "Failed to register DRM driver\n"); @@ -1142,6 +1201,8 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
return 0;
+err_private_state_free: + kfree(private_state); err_clk_notifier_unregister: clk_notifier_unregister(parent_clk, &priv->clock_nb); err_devclk_disable: diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c index aeb8a757d213..c819293b8317 100644 --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c @@ -45,6 +45,10 @@ struct soc_info { unsigned int weight, unsigned int offset); };
+struct ingenic_ipu_private_state { + struct drm_private_state base; +}; + struct ingenic_ipu { struct drm_plane plane; struct drm_device *drm; @@ -60,6 +64,8 @@ struct ingenic_ipu {
struct drm_property *sharpness_prop; unsigned int sharpness; + + struct drm_private_obj private_obj; };
/* Signed 15.16 fixed-point math (for bicubic scaling coefficients) */ @@ -73,6 +79,12 @@ static inline struct ingenic_ipu *plane_to_ingenic_ipu(struct drm_plane *plane) return container_of(plane, struct ingenic_ipu, plane); }
+static inline struct ingenic_ipu_private_state * +to_ingenic_ipu_priv_state(struct drm_private_state *state) +{ + return container_of(state, struct ingenic_ipu_private_state, base); +} + /* * Apply conventional cubic convolution kernel. Both parameters * and return value are 15.16 signed fixed-point. @@ -679,6 +691,33 @@ static const struct drm_plane_funcs ingenic_ipu_plane_funcs = { .atomic_set_property = ingenic_ipu_plane_atomic_set_property, };
+static struct drm_private_state * +ingenic_ipu_duplicate_state(struct drm_private_obj *obj) +{ + struct ingenic_ipu_private_state *state = to_ingenic_ipu_priv_state(obj->state); + + state = kmemdup(state, sizeof(*state), GFP_KERNEL); + if (!state) + return NULL; + + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base); + + return &state->base; +} + +static void ingenic_ipu_destroy_state(struct drm_private_obj *obj, + struct drm_private_state *state) +{ + struct ingenic_ipu_private_state *priv_state = to_ingenic_ipu_priv_state(state); + + kfree(priv_state); +} + +static const struct drm_private_state_funcs ingenic_ipu_private_state_funcs = { + .atomic_duplicate_state = ingenic_ipu_duplicate_state, + .atomic_destroy_state = ingenic_ipu_destroy_state, +}; + static irqreturn_t ingenic_ipu_irq_handler(int irq, void *arg) { struct ingenic_ipu *ipu = arg; @@ -717,6 +756,7 @@ static const struct regmap_config ingenic_ipu_regmap_config = { static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d) { struct platform_device *pdev = to_platform_device(dev); + struct ingenic_ipu_private_state *private_state; const struct soc_info *soc_info; struct drm_device *drm = d; struct drm_plane *plane; @@ -810,7 +850,20 @@ static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d) return err; }
+ private_state = kzalloc(sizeof(*private_state), GFP_KERNEL); + if (!private_state) { + err = -ENOMEM; + goto err_clk_unprepare; + } + + drm_atomic_private_obj_init(drm, &ipu->private_obj, &private_state->base, + &ingenic_ipu_private_state_funcs); + return 0; + +err_clk_unprepare: + clk_unprepare(ipu->clk); + return err; }
static void ingenic_ipu_unbind(struct device *dev, @@ -818,6 +871,7 @@ static void ingenic_ipu_unbind(struct device *dev, { struct ingenic_ipu *ipu = dev_get_drvdata(dev);
+ drm_atomic_private_obj_fini(&ipu->private_obj); clk_unprepare(ipu->clk); }
The IPU scaling information is computed in the plane's ".atomic_check" callback, and used in the ".atomic_update" callback. As such, it is state-specific, and should be moved to a private state structure.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-ipu.c | 73 ++++++++++++++++++++------- 1 file changed, 54 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c index c819293b8317..2737fc521e15 100644 --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c @@ -47,6 +47,8 @@ struct soc_info {
struct ingenic_ipu_private_state { struct drm_private_state base; + + unsigned int num_w, num_h, denom_w, denom_h; };
struct ingenic_ipu { @@ -58,8 +60,6 @@ struct ingenic_ipu { const struct soc_info *soc_info; bool clk_enabled;
- unsigned int num_w, num_h, denom_w, denom_h; - dma_addr_t addr_y, addr_u, addr_v;
struct drm_property *sharpness_prop; @@ -85,6 +85,30 @@ to_ingenic_ipu_priv_state(struct drm_private_state *state) return container_of(state, struct ingenic_ipu_private_state, base); }
+static struct ingenic_ipu_private_state * +ingenic_ipu_get_priv_state(struct ingenic_ipu *priv, struct drm_atomic_state *state) +{ + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj); + if (IS_ERR(priv_state)) + return ERR_CAST(priv_state); + + return to_ingenic_ipu_priv_state(priv_state); +} + +static struct ingenic_ipu_private_state * +ingenic_ipu_get_new_priv_state(struct ingenic_ipu *priv, struct drm_atomic_state *state) +{ + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj); + if (!priv_state) + return NULL; + + return to_ingenic_ipu_priv_state(priv_state); +} + /* * Apply conventional cubic convolution kernel. Both parameters * and return value are 15.16 signed fixed-point. @@ -305,11 +329,16 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane, const struct drm_format_info *finfo; u32 ctrl, stride = 0, coef_index = 0, format = 0; bool needs_modeset, upscaling_w, upscaling_h; + struct ingenic_ipu_private_state *ipu_state; int err;
if (!newstate || !newstate->fb) return;
+ ipu_state = ingenic_ipu_get_new_priv_state(ipu, state); + if (WARN_ON(!ipu_state)) + return; + finfo = drm_format_info(newstate->fb->format->format);
if (!ipu->clk_enabled) { @@ -482,27 +511,27 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane, if (ipu->soc_info->has_bicubic) ctrl |= JZ_IPU_CTRL_ZOOM_SEL;
- upscaling_w = ipu->num_w > ipu->denom_w; + upscaling_w = ipu_state->num_w > ipu_state->denom_w; if (upscaling_w) ctrl |= JZ_IPU_CTRL_HSCALE;
- if (ipu->num_w != 1 || ipu->denom_w != 1) { + if (ipu_state->num_w != 1 || ipu_state->denom_w != 1) { if (!ipu->soc_info->has_bicubic && !upscaling_w) - coef_index |= (ipu->denom_w - 1) << 16; + coef_index |= (ipu_state->denom_w - 1) << 16; else - coef_index |= (ipu->num_w - 1) << 16; + coef_index |= (ipu_state->num_w - 1) << 16; ctrl |= JZ_IPU_CTRL_HRSZ_EN; }
- upscaling_h = ipu->num_h > ipu->denom_h; + upscaling_h = ipu_state->num_h > ipu_state->denom_h; if (upscaling_h) ctrl |= JZ_IPU_CTRL_VSCALE;
- if (ipu->num_h != 1 || ipu->denom_h != 1) { + if (ipu_state->num_h != 1 || ipu_state->denom_h != 1) { if (!ipu->soc_info->has_bicubic && !upscaling_h) - coef_index |= ipu->denom_h - 1; + coef_index |= ipu_state->denom_h - 1; else - coef_index |= ipu->num_h - 1; + coef_index |= ipu_state->num_h - 1; ctrl |= JZ_IPU_CTRL_VRSZ_EN; }
@@ -513,13 +542,13 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane, /* Set the LUT index register */ regmap_write(ipu->map, JZ_REG_IPU_RSZ_COEF_INDEX, coef_index);
- if (ipu->num_w != 1 || ipu->denom_w != 1) + if (ipu_state->num_w != 1 || ipu_state->denom_w != 1) ingenic_ipu_set_coefs(ipu, JZ_REG_IPU_HRSZ_COEF_LUT, - ipu->num_w, ipu->denom_w); + ipu_state->num_w, ipu_state->denom_w);
- if (ipu->num_h != 1 || ipu->denom_h != 1) + if (ipu_state->num_h != 1 || ipu_state->denom_h != 1) ingenic_ipu_set_coefs(ipu, JZ_REG_IPU_VRSZ_COEF_LUT, - ipu->num_h, ipu->denom_h); + ipu_state->num_h, ipu_state->denom_h);
/* Clear STATUS register */ regmap_write(ipu->map, JZ_REG_IPU_STATUS, 0); @@ -531,7 +560,8 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane, dev_dbg(ipu->dev, "Scaling %ux%u to %ux%u (%u:%u horiz, %u:%u vert)\n", newstate->src_w >> 16, newstate->src_h >> 16, newstate->crtc_w, newstate->crtc_h, - ipu->num_w, ipu->denom_w, ipu->num_h, ipu->denom_h); + ipu_state->num_w, ipu_state->denom_w, + ipu_state->num_h, ipu_state->denom_h); }
static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane, @@ -545,6 +575,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane, struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane); struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; struct drm_crtc_state *crtc_state; + struct ingenic_ipu_private_state *ipu_state;
if (!crtc) return 0; @@ -553,6 +584,10 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane, if (WARN_ON(!crtc_state)) return -EINVAL;
+ ipu_state = ingenic_ipu_get_priv_state(ipu, state); + if (IS_ERR(ipu_state)) + return PTR_ERR(ipu_state); + /* Request a full modeset if we are enabling or disabling the IPU. */ if (!old_plane_state->crtc ^ !new_plane_state->crtc) crtc_state->mode_changed = true; @@ -605,10 +640,10 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane, if (num_h > max_h) return -EINVAL;
- ipu->num_w = num_w; - ipu->num_h = num_h; - ipu->denom_w = denom_w; - ipu->denom_h = denom_h; + ipu_state->num_w = num_w; + ipu_state->num_h = num_h; + ipu_state->denom_w = denom_w; + ipu_state->denom_h = denom_h;
out_check_damage: if (ingenic_drm_map_noncoherent(ipu->master))
Setting the DMA descriptor chain register in the probe function has been fine until now, because we only ever had one descriptor per foreground.
As the driver will soon have real descriptor chains, and the DMA descriptor chain register updates itself to point to the current descriptor being processed, this register needs to be reset after a full modeset to point to the first descriptor of the chain.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 3fc01cffb00f..2eef174165a2 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -184,6 +184,10 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
+ /* Set address of our DMA descriptor chain */ + regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0)); + regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1)); + regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE, JZ_LCD_CTRL_ENABLE);
When using C8 color mode, make sure that the palette is always uploaded before a frame; otherwise the very first frame will have wrong colors.
Do that by changing the link order of the DMA descriptors.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 69 +++++++++++++++++++---- 1 file changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 2eef174165a2..7ae48ead3ab6 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -65,6 +65,7 @@ struct jz_soc_info {
struct ingenic_drm_private_state { struct drm_private_state base; + bool use_palette; };
struct ingenic_drm { @@ -112,6 +113,30 @@ to_ingenic_drm_priv_state(struct drm_private_state *state) return container_of(state, struct ingenic_drm_private_state, base); }
+static struct ingenic_drm_private_state * +ingenic_drm_get_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state) +{ + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_private_obj_state(state, &priv->private_obj); + if (IS_ERR(priv_state)) + return ERR_CAST(priv_state); + + return to_ingenic_drm_priv_state(priv_state); +} + +static struct ingenic_drm_private_state * +ingenic_drm_get_new_priv_state(struct ingenic_drm *priv, struct drm_atomic_state *state) +{ + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_new_private_obj_state(state, &priv->private_obj); + if (!priv_state) + return NULL; + + return to_ingenic_drm_priv_state(priv_state); +} + static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg) { switch (reg) { @@ -159,6 +184,13 @@ static inline dma_addr_t dma_hwdesc_addr(const struct ingenic_drm *priv, bool us return priv->dma_hwdescs_phys + offset; }
+static inline dma_addr_t dma_hwdesc_pal_addr(const struct ingenic_drm *priv) +{ + u32 offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal); + + return priv->dma_hwdescs_phys + offset; +} + static int ingenic_drm_update_pixclk(struct notifier_block *nb, unsigned long action, void *data) @@ -181,11 +213,19 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state) { struct ingenic_drm *priv = drm_crtc_get_priv(crtc); + struct ingenic_drm_private_state *priv_state; + + priv_state = ingenic_drm_get_new_priv_state(priv, state); + if (WARN_ON(!priv_state)) + return;
regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
/* Set address of our DMA descriptor chain */ - regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0)); + if (priv_state->use_palette) + regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_pal_addr(priv)); + else + regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, 0)); regmap_write(priv->map, JZ_REG_LCD_DA1, dma_hwdesc_addr(priv, 1));
regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, @@ -391,6 +431,7 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); struct ingenic_drm *priv = drm_device_get_priv(plane->dev); + struct ingenic_drm_private_state *priv_state; struct drm_crtc_state *crtc_state; struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc; int ret; @@ -403,6 +444,10 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane, if (WARN_ON(!crtc_state)) return -EINVAL;
+ priv_state = ingenic_drm_get_priv_state(priv, state); + if (IS_ERR(priv_state)) + return PTR_ERR(priv_state); + ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, DRM_PLANE_HELPER_NO_SCALING, DRM_PLANE_HELPER_NO_SCALING, @@ -421,6 +466,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane, (new_plane_state->src_h >> 16) != new_plane_state->crtc_h)) return -EINVAL;
+ priv_state->use_palette = new_plane_state->fb && + new_plane_state->fb->format->format == DRM_FORMAT_C8; + /* * Require full modeset if enabling or disabling a plane, or changing * its position, size or depth. @@ -580,10 +628,11 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, struct ingenic_drm *priv = drm_device_get_priv(plane->dev); struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state, plane); struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state, plane); + struct ingenic_drm_private_state *priv_state; struct drm_crtc_state *crtc_state; struct ingenic_dma_hwdesc *hwdesc; - unsigned int width, height, cpp, offset; - dma_addr_t addr; + unsigned int width, height, cpp; + dma_addr_t addr, next_addr; bool use_f1; u32 fourcc;
@@ -599,23 +648,23 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, height = newstate->src_h >> 16; cpp = newstate->fb->format->cpp[0];
+ priv_state = ingenic_drm_get_new_priv_state(priv, state); + if (priv_state && priv_state->use_palette) + next_addr = dma_hwdesc_pal_addr(priv); + else + next_addr = dma_hwdesc_addr(priv, use_f1); + hwdesc = &priv->dma_hwdescs->hwdesc[use_f1];
hwdesc->addr = addr; hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4); + hwdesc->next = next_addr;
if (drm_atomic_crtc_needs_modeset(crtc_state)) { fourcc = newstate->fb->format->format;
ingenic_drm_plane_config(priv->dev, plane, fourcc);
- if (fourcc == DRM_FORMAT_C8) - offset = offsetof(struct ingenic_dma_hwdescs, hwdesc_pal); - else - offset = offsetof(struct ingenic_dma_hwdescs, hwdesc[0]); - - priv->dma_hwdescs->hwdesc[0].next = priv->dma_hwdescs_phys + offset; - crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8; }
Attach a top-level bridge to each encoder, which will be used for negociating the bus format and flags.
All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Paul Cercueil paul@crapouillou.net --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------ 1 file changed, 70 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 7ae48ead3ab6..09d5dd298078 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -21,6 +21,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_color_mgmt.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> @@ -107,6 +108,19 @@ struct ingenic_drm { struct drm_private_obj private_obj; };
+struct ingenic_drm_bridge { + struct drm_encoder encoder; + struct drm_bridge bridge, *next_bridge; + + struct drm_bus_cfg bus_cfg; +}; + +static inline struct ingenic_drm_bridge * +to_ingenic_drm_bridge(struct drm_encoder *encoder) +{ + return container_of(encoder, struct ingenic_drm_bridge, encoder); +} + static inline struct ingenic_drm_private_state * to_ingenic_drm_priv_state(struct drm_private_state *state) { @@ -679,11 +693,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, { struct ingenic_drm *priv = drm_device_get_priv(encoder->dev); struct drm_display_mode *mode = &crtc_state->adjusted_mode; - struct drm_connector *conn = conn_state->connector; - struct drm_display_info *info = &conn->display_info; + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder); unsigned int cfg, rgbcfg = 0;
- priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS; + priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
if (priv->panel_is_sharp) { cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY; @@ -696,19 +709,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC) cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW; - if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW) cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW; - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
if (!priv->panel_is_sharp) { - if (conn->connector_type == DRM_MODE_CONNECTOR_TV) { + if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) { if (mode->flags & DRM_MODE_FLAG_INTERLACE) cfg |= JZ_LCD_CFG_MODE_TV_OUT_I; else cfg |= JZ_LCD_CFG_MODE_TV_OUT_P; } else { - switch (*info->bus_formats) { + switch (bridge->bus_cfg.format) { case MEDIA_BUS_FMT_RGB565_1X16: cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT; break; @@ -734,20 +747,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg); }
-static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, - struct drm_crtc_state *crtc_state, - struct drm_connector_state *conn_state) +static int ingenic_drm_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) +{ + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder); + + return drm_bridge_attach(bridge->encoder, ib->next_bridge, + &ib->bridge, flags); +} + +static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) { - struct drm_display_info *info = &conn_state->connector->display_info; struct drm_display_mode *mode = &crtc_state->adjusted_mode; + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
- if (info->num_bus_formats != 1) - return -EINVAL; + ib->bus_cfg = bridge_state->output_bus_cfg;
if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) return 0;
- switch (*info->bus_formats) { + switch (bridge_state->output_bus_cfg.format) { case MEDIA_BUS_FMT_RGB888_3X8: case MEDIA_BUS_FMT_RGB888_3X8_DELTA: /* @@ -911,8 +933,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = { };
static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = { - .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set, - .atomic_check = ingenic_drm_encoder_atomic_check, + .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set, +}; + +static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = { + .attach = ingenic_drm_bridge_attach, + .atomic_check = ingenic_drm_bridge_atomic_check, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt, };
static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { @@ -958,7 +988,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_plane *primary; struct drm_bridge *bridge; struct drm_panel *panel; + struct drm_connector *connector; struct drm_encoder *encoder; + struct ingenic_drm_bridge *ib; struct drm_device *drm; void __iomem *base; long parent_rate; @@ -1146,20 +1178,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) bridge = devm_drm_panel_bridge_add_typed(dev, panel, DRM_MODE_CONNECTOR_DPI);
- encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL); - if (IS_ERR(encoder)) { - ret = PTR_ERR(encoder); + ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder, + NULL, DRM_MODE_ENCODER_DPI, NULL); + if (IS_ERR(ib)) { + ret = PTR_ERR(ib); dev_err(dev, "Failed to init encoder: %d\n", ret); return ret; }
- encoder->possible_crtcs = 1; + encoder = &ib->encoder; + encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
- ret = drm_bridge_attach(encoder, bridge, NULL, 0); - if (ret) + ib->bridge.funcs = &ingenic_drm_bridge_funcs; + ib->next_bridge = bridge; + + ret = drm_bridge_attach(encoder, &ib->bridge, NULL, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret) { + dev_err(dev, "Unable to attach bridge\n"); return ret; + } + + connector = drm_bridge_connector_init(drm, encoder); + if (IS_ERR(connector)) { + dev_err(dev, "Unable to init connector\n"); + return PTR_ERR(connector); + } + + drm_connector_attach_encoder(connector, encoder); }
drm_for_each_encoder(encoder, drm) {
Hi Paul, all other patches apply cleanly but this one fails on top of v5.14-rc4. What base are you using? BR and thanks, Nikolaus
Am 08.08.2021 um 15:45 schrieb Paul Cercueil paul@crapouillou.net:
Attach a top-level bridge to each encoder, which will be used for negociating the bus format and flags.
All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------ 1 file changed, 70 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 7ae48ead3ab6..09d5dd298078 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -21,6 +21,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_color_mgmt.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> @@ -107,6 +108,19 @@ struct ingenic_drm { struct drm_private_obj private_obj; };
+struct ingenic_drm_bridge {
- struct drm_encoder encoder;
- struct drm_bridge bridge, *next_bridge;
- struct drm_bus_cfg bus_cfg;
+};
+static inline struct ingenic_drm_bridge * +to_ingenic_drm_bridge(struct drm_encoder *encoder) +{
- return container_of(encoder, struct ingenic_drm_bridge, encoder);
+}
static inline struct ingenic_drm_private_state * to_ingenic_drm_priv_state(struct drm_private_state *state) { @@ -679,11 +693,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, { struct ingenic_drm *priv = drm_device_get_priv(encoder->dev); struct drm_display_mode *mode = &crtc_state->adjusted_mode;
- struct drm_connector *conn = conn_state->connector;
- struct drm_display_info *info = &conn->display_info;
- struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder); unsigned int cfg, rgbcfg = 0;
- priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
if (priv->panel_is_sharp) { cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
@@ -696,19 +709,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC) cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
- if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW) cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
if (!priv->panel_is_sharp) {
if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) { if (mode->flags & DRM_MODE_FLAG_INTERLACE) cfg |= JZ_LCD_CFG_MODE_TV_OUT_I; else cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
switch (*info->bus_formats) {
switch (bridge->bus_cfg.format) { case MEDIA_BUS_FMT_RGB565_1X16: cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT; break;@@ -734,20 +747,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg); }
-static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state)+static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)+{
- struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
- return drm_bridge_attach(bridge->encoder, ib->next_bridge,
&ib->bridge, flags);+}
+static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
struct drm_bridge_state *bridge_state,struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state){
- struct drm_display_info *info = &conn_state->connector->display_info; struct drm_display_mode *mode = &crtc_state->adjusted_mode;
- struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
- if (info->num_bus_formats != 1)
return -EINVAL;
ib->bus_cfg = bridge_state->output_bus_cfg;
if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) return 0;
- switch (*info->bus_formats) {
- switch (bridge_state->output_bus_cfg.format) { case MEDIA_BUS_FMT_RGB888_3X8: case MEDIA_BUS_FMT_RGB888_3X8_DELTA: /*
@@ -911,8 +933,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = { };
static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
- .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
- .atomic_check = ingenic_drm_encoder_atomic_check,
- .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
+};
+static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
- .attach = ingenic_drm_bridge_attach,
- .atomic_check = ingenic_drm_bridge_atomic_check,
- .atomic_reset = drm_atomic_helper_bridge_reset,
- .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
- .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
};
static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { @@ -958,7 +988,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_plane *primary; struct drm_bridge *bridge; struct drm_panel *panel;
- struct drm_connector *connector; struct drm_encoder *encoder;
- struct ingenic_drm_bridge *ib; struct drm_device *drm; void __iomem *base; long parent_rate;
@@ -1146,20 +1178,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) bridge = devm_drm_panel_bridge_add_typed(dev, panel, DRM_MODE_CONNECTOR_DPI);
encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);if (IS_ERR(encoder)) {ret = PTR_ERR(encoder);
ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,NULL, DRM_MODE_ENCODER_DPI, NULL);if (IS_ERR(ib)) {ret = PTR_ERR(ib); dev_err(dev, "Failed to init encoder: %d\n", ret); return ret;
encoder->possible_crtcs = 1;
encoder = &ib->encoder;encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
ret = drm_bridge_attach(encoder, bridge, NULL, 0);if (ret)
ib->bridge.funcs = &ingenic_drm_bridge_funcs;ib->next_bridge = bridge;ret = drm_bridge_attach(encoder, &ib->bridge, NULL,DRM_BRIDGE_ATTACH_NO_CONNECTOR);if (ret) {dev_err(dev, "Unable to attach bridge\n"); return ret;}connector = drm_bridge_connector_init(drm, encoder);if (IS_ERR(connector)) {dev_err(dev, "Unable to init connector\n");return PTR_ERR(connector);}drm_connector_attach_encoder(connector, encoder);}
drm_for_each_encoder(encoder, drm) {
-- 2.30.2
Hi Nikolaus,
Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :
Hi Paul, all other patches apply cleanly but this one fails on top of v5.14-rc4. What base are you using? BR and thanks, Nikolaus
The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
Cheers, -Paul
Am 08.08.2021 um 15:45 schrieb Paul Cercueil paul@crapouillou.net:
Attach a top-level bridge to each encoder, which will be used for negociating the bus format and flags.
All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------ 1 file changed, 70 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 7ae48ead3ab6..09d5dd298078 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -21,6 +21,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_color_mgmt.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> @@ -107,6 +108,19 @@ struct ingenic_drm { struct drm_private_obj private_obj; };
+struct ingenic_drm_bridge {
- struct drm_encoder encoder;
- struct drm_bridge bridge, *next_bridge;
- struct drm_bus_cfg bus_cfg;
+};
+static inline struct ingenic_drm_bridge * +to_ingenic_drm_bridge(struct drm_encoder *encoder) +{
- return container_of(encoder, struct ingenic_drm_bridge, encoder);
+}
static inline struct ingenic_drm_private_state * to_ingenic_drm_priv_state(struct drm_private_state *state) { @@ -679,11 +693,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, { struct ingenic_drm *priv = drm_device_get_priv(encoder->dev); struct drm_display_mode *mode = &crtc_state->adjusted_mode;
- struct drm_connector *conn = conn_state->connector;
- struct drm_display_info *info = &conn->display_info;
- struct ingenic_drm_bridge *bridge =
to_ingenic_drm_bridge(encoder); unsigned int cfg, rgbcfg = 0;
- priv->panel_is_sharp = info->bus_flags &
DRM_BUS_FLAG_SHARP_SIGNALS;
- priv->panel_is_sharp = bridge->bus_cfg.flags &
DRM_BUS_FLAG_SHARP_SIGNALS;
if (priv->panel_is_sharp) { cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY; @@ -696,19 +709,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC) cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
- if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW) cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
if (!priv->panel_is_sharp) {
if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
if (conn_state->connector->connector_type ==DRM_MODE_CONNECTOR_TV) { if (mode->flags & DRM_MODE_FLAG_INTERLACE) cfg |= JZ_LCD_CFG_MODE_TV_OUT_I; else cfg |= JZ_LCD_CFG_MODE_TV_OUT_P; } else {
switch (*info->bus_formats) {
switch (bridge->bus_cfg.format) { case MEDIA_BUS_FMT_RGB565_1X16: cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT; break;@@ -734,20 +747,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg); }
-static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state)+static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)+{
- struct ingenic_drm_bridge *ib =
to_ingenic_drm_bridge(bridge->encoder);
- return drm_bridge_attach(bridge->encoder, ib->next_bridge,
&ib->bridge, flags);+}
+static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
struct drm_bridge_state *bridge_state,struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state){
- struct drm_display_info *info =
&conn_state->connector->display_info; struct drm_display_mode *mode = &crtc_state->adjusted_mode;
- struct ingenic_drm_bridge *ib =
to_ingenic_drm_bridge(bridge->encoder);
- if (info->num_bus_formats != 1)
return -EINVAL;
ib->bus_cfg = bridge_state->output_bus_cfg;
if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) return 0;
- switch (*info->bus_formats) {
- switch (bridge_state->output_bus_cfg.format) { case MEDIA_BUS_FMT_RGB888_3X8: case MEDIA_BUS_FMT_RGB888_3X8_DELTA: /*
@@ -911,8 +933,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = { };
static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
- .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
- .atomic_check = ingenic_drm_encoder_atomic_check,
- .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
+};
+static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
- .attach = ingenic_drm_bridge_attach,
- .atomic_check = ingenic_drm_bridge_atomic_check,
- .atomic_reset = drm_atomic_helper_bridge_reset,
- .atomic_duplicate_state =
drm_atomic_helper_bridge_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
- .atomic_get_input_bus_fmts =
drm_atomic_helper_bridge_propagate_bus_fmt, };
static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { @@ -958,7 +988,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_plane *primary; struct drm_bridge *bridge; struct drm_panel *panel;
- struct drm_connector *connector; struct drm_encoder *encoder;
- struct ingenic_drm_bridge *ib; struct drm_device *drm; void __iomem *base; long parent_rate;
@@ -1146,20 +1178,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) bridge = devm_drm_panel_bridge_add_typed(dev, panel, DRM_MODE_CONNECTOR_DPI);
encoder = drmm_plain_encoder_alloc(drm, NULL,DRM_MODE_ENCODER_DPI, NULL);
if (IS_ERR(encoder)) {ret = PTR_ERR(encoder);
ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,NULL, DRM_MODE_ENCODER_DPI, NULL);if (IS_ERR(ib)) {ret = PTR_ERR(ib); dev_err(dev, "Failed to init encoder: %d\n", ret); return ret;
encoder->possible_crtcs = 1;
encoder = &ib->encoder;encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);drm_encoder_helper_add(encoder,
&ingenic_drm_encoder_helper_funcs);
ret = drm_bridge_attach(encoder, bridge, NULL, 0);if (ret)
ib->bridge.funcs = &ingenic_drm_bridge_funcs;ib->next_bridge = bridge;ret = drm_bridge_attach(encoder, &ib->bridge, NULL,DRM_BRIDGE_ATTACH_NO_CONNECTOR);if (ret) {dev_err(dev, "Unable to attach bridge\n"); return ret;}connector = drm_bridge_connector_init(drm, encoder);if (IS_ERR(connector)) {dev_err(dev, "Unable to init connector\n");return PTR_ERR(connector);}drm_connector_attach_encoder(connector, encoder);}
drm_for_each_encoder(encoder, drm) {
-- 2.30.2
Am 08.08.2021 um 21:04 schrieb Paul Cercueil paul@crapouillou.net:
Hi Nikolaus,
Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :
Hi Paul, all other patches apply cleanly but this one fails on top of v5.14-rc4. What base are you using? BR and thanks, Nikolaus
The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
Ok, fine!
BR and thanks, Nikolaus
Cheers, -Paul
Am 08.08.2021 um 15:45 schrieb Paul Cercueil paul@crapouillou.net: Attach a top-level bridge to each encoder, which will be used for negociating the bus format and flags. All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR. Signed-off-by: Paul Cercueil paul@crapouillou.net
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------ 1 file changed, 70 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index 7ae48ead3ab6..09d5dd298078 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -21,6 +21,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_color_mgmt.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> @@ -107,6 +108,19 @@ struct ingenic_drm { struct drm_private_obj private_obj; }; +struct ingenic_drm_bridge {
- struct drm_encoder encoder;
- struct drm_bridge bridge, *next_bridge;
- struct drm_bus_cfg bus_cfg;
+};
+static inline struct ingenic_drm_bridge * +to_ingenic_drm_bridge(struct drm_encoder *encoder) +{
- return container_of(encoder, struct ingenic_drm_bridge, encoder);
+}
static inline struct ingenic_drm_private_state * to_ingenic_drm_priv_state(struct drm_private_state *state) { @@ -679,11 +693,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, { struct ingenic_drm *priv = drm_device_get_priv(encoder->dev); struct drm_display_mode *mode = &crtc_state->adjusted_mode;
- struct drm_connector *conn = conn_state->connector;
- struct drm_display_info *info = &conn->display_info;
- struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder); unsigned int cfg, rgbcfg = 0;
- priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
- priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS; if (priv->panel_is_sharp) { cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY;
@@ -696,19 +709,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC) cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_DE_LOW)
- if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW) cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
- if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
- if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE; if (!priv->panel_is_sharp) {
if (conn->connector_type == DRM_MODE_CONNECTOR_TV) {
if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) { if (mode->flags & DRM_MODE_FLAG_INTERLACE) cfg |= JZ_LCD_CFG_MODE_TV_OUT_I; else cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
switch (*info->bus_formats) {
switch (bridge->bus_cfg.format) { case MEDIA_BUS_FMT_RGB565_1X16: cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT; break;@@ -734,20 +747,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg); } -static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state)+static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)+{
- struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
- return drm_bridge_attach(bridge->encoder, ib->next_bridge,
&ib->bridge, flags);+}
+static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
struct drm_bridge_state *bridge_state,struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state){
- struct drm_display_info *info = &conn_state->connector->display_info; struct drm_display_mode *mode = &crtc_state->adjusted_mode;
- struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
- if (info->num_bus_formats != 1)
return -EINVAL;
- ib->bus_cfg = bridge_state->output_bus_cfg; if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) return 0;
- switch (*info->bus_formats) {
- switch (bridge_state->output_bus_cfg.format) { case MEDIA_BUS_FMT_RGB888_3X8: case MEDIA_BUS_FMT_RGB888_3X8_DELTA: /*
@@ -911,8 +933,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = { }; static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = {
- .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
- .atomic_check = ingenic_drm_encoder_atomic_check,
- .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set,
+};
+static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = {
- .attach = ingenic_drm_bridge_attach,
- .atomic_check = ingenic_drm_bridge_atomic_check,
- .atomic_reset = drm_atomic_helper_bridge_reset,
- .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
- .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt,
}; static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { @@ -958,7 +988,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_plane *primary; struct drm_bridge *bridge; struct drm_panel *panel;
- struct drm_connector *connector; struct drm_encoder *encoder;
- struct ingenic_drm_bridge *ib; struct drm_device *drm; void __iomem *base; long parent_rate;
@@ -1146,20 +1178,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) bridge = devm_drm_panel_bridge_add_typed(dev, panel, DRM_MODE_CONNECTOR_DPI);
encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);if (IS_ERR(encoder)) {ret = PTR_ERR(encoder);
ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,NULL, DRM_MODE_ENCODER_DPI, NULL);if (IS_ERR(ib)) {ret = PTR_ERR(ib); dev_err(dev, "Failed to init encoder: %d\n", ret); return ret;
encoder->possible_crtcs = 1;
encoder = &ib->encoder;encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
ret = drm_bridge_attach(encoder, bridge, NULL, 0);if (ret)
ib->bridge.funcs = &ingenic_drm_bridge_funcs;ib->next_bridge = bridge;ret = drm_bridge_attach(encoder, &ib->bridge, NULL,DRM_BRIDGE_ATTACH_NO_CONNECTOR);if (ret) {dev_err(dev, "Unable to attach bridge\n"); return ret;}connector = drm_bridge_connector_init(drm, encoder);if (IS_ERR(connector)) {dev_err(dev, "Unable to init connector\n");return PTR_ERR(connector);}drm_connector_attach_encoder(connector, encoder);-- 2.30.2
Am 08.08.2021 um 21:06 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 08.08.2021 um 21:04 schrieb Paul Cercueil paul@crapouillou.net:
Hi Nikolaus,
Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :
Hi Paul, all other patches apply cleanly but this one fails on top of v5.14-rc4. What base are you using? BR and thanks, Nikolaus
The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
Ok, fine!
Contains 3 patches for drm/ingenic and after taking them first, I can apply the series.
Again, BR and thanks, Nikolaus
Hi Paul, quick feedback: our HDMI on top compiles fine after fixing 2 merge conflicts, but dos not yet work. Will need some spare time with access to the CI20 board to research the issue, i.e. can not give feedback immediately. BR and thanks, Nikolaus
Am 08.08.2021 um 21:12 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 08.08.2021 um 21:06 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 08.08.2021 um 21:04 schrieb Paul Cercueil paul@crapouillou.net:
Hi Nikolaus,
Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :
Hi Paul, all other patches apply cleanly but this one fails on top of v5.14-rc4. What base are you using? BR and thanks, Nikolaus
The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
Ok, fine!
Contains 3 patches for drm/ingenic and after taking them first, I can apply the series.
Again, BR and thanks, Nikolaus
https://projects.goldelico.com/p/gta04-kernel/ Letux-kernel mailing list Letux-kernel@openphoenux.org http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
Hi Nikolaus,
Le lun., août 9 2021 at 13:14:03 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :
Hi Paul, quick feedback: our HDMI on top compiles fine after fixing 2 merge conflicts, but dos not yet work. Will need some spare time with access to the CI20 board to research the issue, i.e. can not give feedback immediately.
Alright, no problem. I'll be back home in about 2 weeks and then I can test on my CI20 as well.
Cheers, -Paul
BR and thanks, Nikolaus
Am 08.08.2021 um 21:12 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 08.08.2021 um 21:06 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 08.08.2021 um 21:04 schrieb Paul Cercueil paul@crapouillou.net:
Hi Nikolaus,
Le dim., août 8 2021 at 20:57:09 +0200, H. Nikolaus Schaller hns@goldelico.com a écrit :
Hi Paul, all other patches apply cleanly but this one fails on top of v5.14-rc4. What base are you using? BR and thanks, Nikolaus
The base is drm-misc (https://cgit.freedesktop.org/drm/drm-misc), branch drm-misc-next.
Ok, fine!
Contains 3 patches for drm/ingenic and after taking them first, I can apply the series.
Again, BR and thanks, Nikolaus
https://projects.goldelico.com/p/gta04-kernel/ Letux-kernel mailing list Letux-kernel@openphoenux.org http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel
On Monday, 9 August 2021 18:22:12 CEST Paul Cercueil wrote:
Le lun., août 9 2021 at 13:14:03 +0200, H. Nikolaus Schaller
hns@goldelico.com a écrit :
quick feedback: our HDMI on top compiles fine after fixing 2 merge conflicts, but dos not yet work. Will need some spare time with access to the CI20 board to research the issue, i.e. can not give feedback immediately.
Alright, no problem. I'll be back home in about 2 weeks and then I can test on my CI20 as well.
Just for reference, I looked into this initialisation failure. The HDMI peripheral driver gets initialised satisfactorily...
dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY) dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver
But then the reported error occurs in the DRM driver:
ingenic-drm 13050000.lcdc0: Unable to init connector ingenic-drm: probe of 13050000.lcdc0 failed with error -22
This originates in a call to drm_bridge_connector_init from ingenic_drm_bind:
connector = drm_bridge_connector_init(drm, encoder);
The invoked function iterates over the registered bridges, one of which seems to be the HDMI peripheral (it has bridge operations defined identically to those specified in the Synopsys driver), but the type member of the drm_bridge structure is set to 0 (DRM_MODE_CONNECTOR_Unknown).
I might expect the bridge to expose a type acquired from its connector, but I don't see this propagation occurring in the Synopsys driver: dw_hdmi_probe sets the bridge operations and other members of the drm_bridge structure, but it doesn't set the type.
Also, it might be possible that dw_hdmi_connector_detect (exposed as the detect operation) is not getting called, and this would explain why the bridge's connector member does not have the connector_type set, either (since it is also set to 0).
Paul
Hi Paul,
Le mar., août 10 2021 at 01:17:20 +0200, Paul Boddie paul@boddie.org.uk a écrit :
On Monday, 9 August 2021 18:22:12 CEST Paul Cercueil wrote:
Le lun., août 9 2021 at 13:14:03 +0200, H. Nikolaus Schaller
hns@goldelico.com a écrit :
quick feedback: our HDMI on top compiles fine after fixing 2 merge conflicts, but dos not yet work. Will need some spare time with access to the CI20 board to
research
the issue, i.e. can not give feedback immediately.
Alright, no problem. I'll be back home in about 2 weeks and then I can test on my CI20 as well.
Just for reference, I looked into this initialisation failure. The HDMI peripheral driver gets initialised satisfactorily...
dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY) dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver
But then the reported error occurs in the DRM driver:
ingenic-drm 13050000.lcdc0: Unable to init connector ingenic-drm: probe of 13050000.lcdc0 failed with error -22
This originates in a call to drm_bridge_connector_init from ingenic_drm_bind:
connector = drm_bridge_connector_init(drm, encoder);
The invoked function iterates over the registered bridges, one of which seems to be the HDMI peripheral (it has bridge operations defined identically to those specified in the Synopsys driver), but the type member of the drm_bridge structure is set to 0 (DRM_MODE_CONNECTOR_Unknown).
I might expect the bridge to expose a type acquired from its connector, but I don't see this propagation occurring in the Synopsys driver: dw_hdmi_probe sets the bridge operations and other members of the drm_bridge structure, but it doesn't set the type.
Also, it might be possible that dw_hdmi_connector_detect (exposed as the detect operation) is not getting called, and this would explain why the bridge's connector member does not have the connector_type set, either (since it is also set to 0).
From what I understand the last bridge in the chained list is supposed to set the connector type. The HDMI driver's probe function should get a pointer to the next bridge in the queue and attach it (see how ite-it66121.c does it). The last bridge in the queue should be "hdmi-connector" (display-connector.c) which will effectively set the connector type.
Cheers, -Paul
On Tuesday, 10 August 2021 09:52:36 CEST Paul Cercueil wrote:
Le mar., août 10 2021 at 01:17:20 +0200, Paul Boddie paul@boddie.org.uk a
écrit :
But then the reported error occurs in the DRM driver:
ingenic-drm 13050000.lcdc0: Unable to init connector ingenic-drm: probe of 13050000.lcdc0 failed with error -22
This originates in a call to drm_bridge_connector_init from ingenic_drm_bind:
connector = drm_bridge_connector_init(drm, encoder);
The invoked function iterates over the registered bridges, one of which seems to be the HDMI peripheral (it has bridge operations defined identically to those specified in the Synopsys driver), but the type member of the drm_bridge structure is set to 0 (DRM_MODE_CONNECTOR_Unknown).
Although I had fancy ideas about finding the connector node in the device tree and populating the necessary type details on the bridge, I decided to just add the following to the Synopsys driver's dw_hdmi_probe function:
hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
This then lets the above invocation succeed and the Ingenic DRM driver initialises. However, I get "Input Not Supported" on my display.
Conveniently, when indicating the necessary boot arguments...
env set bootargs ... drm.debug=0x3f
...as suggested to me on a previous occasion, the /sys/kernel/debug/dri/0/ state file indicates the following:
plane[31]: plane-0 crtc=crtc-0 fb=36 allocated by = Xorg refcount=2 format=XR24 little-endian (0x34325258) modifier=0x0 size=1280x1024 layers: size[0]=1280x1024 pitch[0]=5120 offset[0]=0 obj[0]: name=0 refcount=3 start=00010000 size=5242880 imported=no paddr=0x00c00000 vaddr=3eb0c080 crtc-pos=1280x1024+0+0 src-pos=1280.000000x1024.000000+0.000000+0.000000 rotation=1 normalized-zpos=0 color-encoding=ITU-R BT.601 YCbCr color-range=YCbCr limited range plane[33]: plane-1 crtc=(null) fb=0 crtc-pos=0x0+0+0 src-pos=0.000000x0.000000+0.000000+0.000000 rotation=1 normalized-zpos=0 color-encoding=ITU-R BT.601 YCbCr color-range=YCbCr limited range crtc[32]: crtc-0 enable=1 active=1 self_refresh_active=0 planes_changed=0 mode_changed=0 active_changed=0 connectors_changed=0 color_mgmt_changed=0 plane_mask=1 connector_mask=1 encoder_mask=1 mode: "": 60 108000 1280 1328 1440 1688 1024 1025 1028 1066 0x0 0x5 connector[35]: HDMI-A-1 crtc=crtc-0 self_refresh_aware=0
I suspect that we may be dealing with an incompatible bus format again, but I may be quite wrong about that, too.
Here is the result of running modetest using...
sudo ./modetest -D /dev/dri/card0 -M ingenic-drm
Encoders: id crtc type possible crtcs possible clones 34 32 DPI 0x00000001 0x00000001
Connectors: id encoder status name size (mm) modes encoders 35 34 connected HDMI-A-1 340x270 17 34 modes: index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot) #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, pvsync; type: preferred, driver #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags: phsync, pvsync; type: driver #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 flags: phsync, pvsync; type: driver #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync, pvsync; type: driver #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync, pvsync; type: driver #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync, nvsync; type: driver #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync, nvsync; type: driver #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync, nvsync; type: driver #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync, pvsync; type: driver #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: phsync, pvsync; type: driver #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: phsync, pvsync; type: driver #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: phsync, pvsync; type: driver #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: nhsync, nvsync; type: driver #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: nhsync, nvsync; type: driver #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: nhsync, nvsync; type: driver #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: nhsync, nvsync; type: driver #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: nhsync, pvsync; type: driver props: 1 EDID: flags: immutable blob blobs:
value: 00ffffffffffff00047232ad01010101 2d0e010380221b782aaea5a6544c9926 145054bfef0081808140714f01010101 010101010101302a009851002a403070 1300520e1100001e000000ff00343435 3030353444454330300a000000fc0041 4c313731350a202020202020000000fd 00384c1e520e000a2020202020200051 2 DPMS: flags: enum enums: On=0 Standby=1 Suspend=2 Off=3 value: 3 5 link-status: flags: enum enums: Good=0 Bad=1 value: 0 6 non-desktop: flags: immutable range values: 0 1 value: 0 4 TILE: flags: immutable blob blobs:
value: 20 CRTC_ID: flags: object value: 32
CRTCs: id fb pos size 32 36 (0,0) (1280x1024) #0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, pvsync; type: props: 22 ACTIVE: flags: range values: 0 1 value: 0 23 MODE_ID: flags: blob blobs:
value: e0a5010000053005a005980600000004 010404042a0400003c00000005000000 00000000000000000000000000000000 00000000000000000000000000000000 00000000 19 OUT_FENCE_PTR: flags: range values: 0 18446744073709551615 value: 0 24 VRR_ENABLED: flags: range values: 0 1 value: 0 28 GAMMA_LUT: flags: blob blobs:
value: 29 GAMMA_LUT_SIZE: flags: immutable range values: 0 4294967295 value: 256
Planes: id crtc fb CRTC x,y x,y gamma size possible crtcs 31 32 36 0,0 0,0 0 0x00000001 formats: XR15 RG16 RG24 XR24 XR30 props: 8 type: flags: immutable enum enums: Overlay=0 Primary=1 Cursor=2 value: 1 17 FB_ID: flags: object value: 36 18 IN_FENCE_FD: flags: signed range values: -1 2147483647 value: -1 20 CRTC_ID: flags: object value: 32 13 CRTC_X: flags: signed range values: -2147483648 2147483647 value: 0 14 CRTC_Y: flags: signed range values: -2147483648 2147483647 value: 0 15 CRTC_W: flags: range values: 0 2147483647 value: 1280 16 CRTC_H: flags: range values: 0 2147483647 value: 1024 9 SRC_X: flags: range values: 0 4294967295 value: 0 10 SRC_Y: flags: range values: 0 4294967295 value: 0 11 SRC_W: flags: range values: 0 4294967295 value: 83886080 12 SRC_H: flags: range values: 0 4294967295 value: 67108864 33 0 0 0,0 0,0 0 0x00000001 formats: C8 XR15 RG16 RG24 XR24 XR30 props: 8 type: flags: immutable enum enums: Overlay=0 Primary=1 Cursor=2 value: 0 17 FB_ID: flags: object value: 0 18 IN_FENCE_FD: flags: signed range values: -1 2147483647 value: -1 20 CRTC_ID: flags: object value: 0 13 CRTC_X: flags: signed range values: -2147483648 2147483647 value: 0 14 CRTC_Y: flags: signed range values: -2147483648 2147483647 value: 0 15 CRTC_W: flags: range values: 0 2147483647 value: 0 16 CRTC_H: flags: range values: 0 2147483647 value: 0 9 SRC_X: flags: range values: 0 4294967295 value: 0 10 SRC_Y: flags: range values: 0 4294967295 value: 0 11 SRC_W: flags: range values: 0 4294967295 value: 0 12 SRC_H: flags: range values: 0 4294967295 value: 0
Frame buffers: id size pitch
Just in case that means anything to anyone.
Paul
Hi Paul, [...]
Encoders: id crtc type possible crtcs possible clones 34 32 DPI 0x00000001 0x00000001
Connectors: id encoder status name size (mm) modes encoders 35 34 connected HDMI-A-1 340x270 17 34 modes: index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot) #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, pvsync; type: preferred, driver #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags: phsync, pvsync; type: driver #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 flags: phsync, pvsync; type: driver #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync, pvsync; type: driver #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync, pvsync; type: driver #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync, nvsync; type: driver #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync, nvsync; type: driver #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync, nvsync; type: driver #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync, pvsync; type: driver #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: phsync, pvsync; type: driver #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: phsync, pvsync; type: driver #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: phsync, pvsync; type: driver #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: nhsync, nvsync; type: driver #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: nhsync, nvsync; type: driver #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: nhsync, nvsync; type: driver #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: nhsync, nvsync; type: driver #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: nhsync, pvsync; type: driver props: 1 EDID: flags: immutable blob blobs:
value: 00ffffffffffff00047232ad01010101 2d0e010380221b782aaea5a6544c9926 145054bfef0081808140714f01010101 010101010101302a009851002a403070 1300520e1100001e000000ff00343435 3030353444454330300a000000fc0041 4c313731350a202020202020000000fd 00384c1e520e000a2020202020200051 2 DPMS: flags: enum enums: On=0 Standby=1 Suspend=2 Off=3 value: 3 5 link-status: flags: enum enums: Good=0 Bad=1 value: 0 6 non-desktop: flags: immutable range values: 0 1 value: 0 4 TILE: flags: immutable blob blobs: value: 20 CRTC_ID: flags: object value: 32CRTCs: id fb pos size 32 36 (0,0) (1280x1024) #0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, pvsync; type: props: 22 ACTIVE: flags: range values: 0 1 value: 0 23 MODE_ID: flags: blob blobs:
value: e0a5010000053005a005980600000004 010404042a0400003c00000005000000 00000000000000000000000000000000 00000000000000000000000000000000 00000000 19 OUT_FENCE_PTR: flags: range values: 0 18446744073709551615 value: 0 24 VRR_ENABLED: flags: range values: 0 1 value: 0 28 GAMMA_LUT: flags: blob blobs: value: 29 GAMMA_LUT_SIZE: flags: immutable range values: 0 4294967295 value: 256Planes: id crtc fb CRTC x,y x,y gamma size possible crtcs 31 32 36 0,0 0,0 0 0x00000001 formats: XR15 RG16 RG24 XR24 XR30 props: 8 type: flags: immutable enum enums: Overlay=0 Primary=1 Cursor=2 value: 1 17 FB_ID: flags: object value: 36 18 IN_FENCE_FD: flags: signed range values: -1 2147483647 value: -1 20 CRTC_ID: flags: object value: 32 13 CRTC_X: flags: signed range values: -2147483648 2147483647 value: 0 14 CRTC_Y: flags: signed range values: -2147483648 2147483647 value: 0 15 CRTC_W: flags: range values: 0 2147483647 value: 1280 16 CRTC_H: flags: range values: 0 2147483647 value: 1024 9 SRC_X: flags: range values: 0 4294967295 value: 0 10 SRC_Y: flags: range values: 0 4294967295 value: 0 11 SRC_W: flags: range values: 0 4294967295 value: 83886080 12 SRC_H: flags: range values: 0 4294967295 value: 67108864 33 0 0 0,0 0,0 0 0x00000001 formats: C8 XR15 RG16 RG24 XR24 XR30 props: 8 type: flags: immutable enum enums: Overlay=0 Primary=1 Cursor=2 value: 0 17 FB_ID: flags: object value: 0 18 IN_FENCE_FD: flags: signed range values: -1 2147483647 value: -1 20 CRTC_ID: flags: object value: 0 13 CRTC_X: flags: signed range values: -2147483648 2147483647 value: 0 14 CRTC_Y: flags: signed range values: -2147483648 2147483647 value: 0 15 CRTC_W: flags: range values: 0 2147483647 value: 0 16 CRTC_H: flags: range values: 0 2147483647 value: 0 9 SRC_X: flags: range values: 0 4294967295 value: 0 10 SRC_Y: flags: range values: 0 4294967295 value: 0 11 SRC_W: flags: range values: 0 4294967295 value: 0 12 SRC_H: flags: range values: 0 4294967295 value: 0
Frame buffers: id size pitch
Just in case that means anything to anyone.
Everything looks good to me. Maybe add some debug in ingenic-drm to see what bus flags and format it ends up choosing.
Cheers, -Paul
dri-devel@lists.freedesktop.org
-
Christophe JAILLET -
Daniel Vetter -
H. Nikolaus Schaller -
Joe Perches -
Paul Boddie -
Paul Cercueil -
Thomas Zimmermann