[PATCH 0/8] gpu/drm: ingenic-drm: Various improvements

[PATCH 0/8] gpu/drm: ingenic-drm: Various improvements

[Paul Cercueil]

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(-)

-- 
2.30.2

[PATCH 1/8] drm/ingenic: Remove dead code

[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>
---
 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);
-- 
2.30.2

Re: [PATCH 1/8] drm/ingenic: Remove dead code

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1329 bytes --]



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);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/8] drm/ingenic: Remove dead code

[Joe Perches]

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);
> > 
> 

Re: [PATCH 1/8] drm/ingenic: Remove dead code

[Paul Cercueil]

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);
>>  >
>> 
> 
> 

[PATCH 2/8] drm/ingenic: Simplify code by using hwdescs array

[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)
+{
+	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);
-- 
2.30.2

Re: [PATCH 2/8] drm/ingenic: Simplify code by using hwdescs array

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5260 bytes --]

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;
> -		else
> -			hwdesc = &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


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/8] drm/ingenic: Simplify code by using hwdescs array

[Paul Cercueil]

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);
>>   \x7f  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);
>>   }
>>   \x7f+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;
>>   \x7f  	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);
>>   \x7f  		crtc_state = newstate->crtc->state;
>> +		use_f1 = priv->soc_info->has_osd && plane != &priv->f0;
>>   \x7f  		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];
>>   \x7f-		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];
> 
> Maybe add a helper that looks up the hwdesc field for a given plane?

Sure.

>>   \x7f  		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]);
>>   \x7f-			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().
> 
>>   \x7f  			crtc_state->color_mgmt_changed = fourcc == DRM_FORMAT_C8;
>>   		}
>> @@ -964,20 +969,17 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>   \x7f  \x7f  	/* 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;
>>   \x7f  	/* 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;
>>   \x7f  	/* 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

[PATCH 3/8] drm/ingenic: Use standard drm_atomic_helper_commit_tail

[Paul Cercueil]

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)
-- 
2.30.2

Re: [PATCH 3/8] drm/ingenic: Use standard drm_atomic_helper_commit_tail

[Christophe JAILLET]

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

Re: [PATCH 3/8] drm/ingenic: Use standard drm_atomic_helper_commit_tail

[Paul Cercueil]

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,
>>   	}
>>   }
>>   \x7f-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.

> Just my 2v.
> CJ

Cheers,
-Paul

Re: [PATCH 3/8] drm/ingenic: Use standard drm_atomic_helper_commit_tail

[Christophe JAILLET]

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,
>>>       }
>>>   }
>>>   \x7f-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
> 
> 
> 

Re: [PATCH 3/8] drm/ingenic: Use standard drm_atomic_helper_commit_tail

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/8] drm/ingenic: Use standard drm_atomic_helper_commit_tail

[Paul Cercueil]

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 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>

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

[PATCH 4/8] drm/ingenic: Add support for private objects

[Paul Cercueil]

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);
 }
 
-- 
2.30.2

[PATCH 5/8] drm/ingenic: Move IPU scale settings to private state

[Paul Cercueil]

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))
-- 
2.30.2

[PATCH 6/8] drm/ingenic: Set DMA descriptor chain register when starting CRTC

[Paul Cercueil]

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);
-- 
2.30.2

[PATCH 7/8] drm/ingenic: Upload palette before frame

[Paul Cercueil]

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;
 		}
 
-- 
2.30.2

[PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

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

Re: [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

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;
> 		} 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
> 

Re: [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

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
>> 
> 

Re: [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

> 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;
>>> 		} 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
> 
> 

Re: [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

> 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

Re: [Letux-kernel] [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

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

Re: [Letux-kernel] [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

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
> 

Re: [Letux-kernel] [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[Paul Boddie]

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

Re: [Letux-kernel] [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

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

Re: [Letux-kernel] [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[Paul Boddie]

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

Re: [Letux-kernel] [PATCH 8/8] drm/ingenic: Attach bridge chain to encoders

[Paul Cercueil]

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: 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.

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