linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] drm/ingenic: IPU plane fixes
@ 2021-03-29 17:50 Paul Cercueil
  2021-03-29 17:50 ` [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY Paul Cercueil
  2021-03-29 17:50 ` [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified Paul Cercueil
  0 siblings, 2 replies; 8+ messages in thread
From: Paul Cercueil @ 2021-03-29 17:50 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Simon Ser
  Cc: Sam Ravnborg, od, linux-mips, dri-devel, linux-kernel, Paul Cercueil

Hi,

A set of two fixes for the IPU plane of the ingenic-drm driver.

Patch [1/2] changes the type of the IPU plane from PRIMARY to OVERLAY,
since there can only be one PRIMARY plane per CRTC.

Patch [2/2] enforces that a full modeset is only performed when the
"sharpness" property is modified.

Cheers,
-Paul

Paul Cercueil (2):
  drm/ingenic: Switch IPU plane to type OVERLAY
  drm/ingenic: Don't request full modeset if property is not modified

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++------
 drivers/gpu/drm/ingenic/ingenic-ipu.c     |  6 ++++--
 2 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY
  2021-03-29 17:50 [PATCH 0/2] drm/ingenic: IPU plane fixes Paul Cercueil
@ 2021-03-29 17:50 ` Paul Cercueil
  2021-03-30  7:23   ` Simon Ser
  2021-03-29 17:50 ` [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified Paul Cercueil
  1 sibling, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2021-03-29 17:50 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Simon Ser
  Cc: Sam Ravnborg, od, linux-mips, dri-devel, linux-kernel,
	Paul Cercueil, stable

It should have been an OVERLAY from the beginning. The documentation
stipulates that there should be an unique PRIMARY plane per CRTC.

Fixes: fc1acf317b01 ("drm/ingenic: Add support for the IPU")
Cc: <stable@vger.kernel.org> # 5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++------
 drivers/gpu/drm/ingenic/ingenic-ipu.c     |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 29742ec5ab95..09225b770bb8 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -419,7 +419,7 @@ static void ingenic_drm_plane_enable(struct ingenic_drm *priv,
 	unsigned int en_bit;
 
 	if (priv->soc_info->has_osd) {
-		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+		if (plane != &priv->f0)
 			en_bit = JZ_LCD_OSDC_F1EN;
 		else
 			en_bit = JZ_LCD_OSDC_F0EN;
@@ -434,7 +434,7 @@ void ingenic_drm_plane_disable(struct device *dev, struct drm_plane *plane)
 	unsigned int en_bit;
 
 	if (priv->soc_info->has_osd) {
-		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+		if (plane != &priv->f0)
 			en_bit = JZ_LCD_OSDC_F1EN;
 		else
 			en_bit = JZ_LCD_OSDC_F0EN;
@@ -461,8 +461,7 @@ void ingenic_drm_plane_config(struct device *dev,
 
 	ingenic_drm_plane_enable(priv, plane);
 
-	if (priv->soc_info->has_osd &&
-	    plane->type == DRM_PLANE_TYPE_PRIMARY) {
+	if (priv->soc_info->has_osd && plane != &priv->f0) {
 		switch (fourcc) {
 		case DRM_FORMAT_XRGB1555:
 			ctrl |= JZ_LCD_OSDCTRL_RGB555;
@@ -510,7 +509,7 @@ void ingenic_drm_plane_config(struct device *dev,
 	}
 
 	if (priv->soc_info->has_osd) {
-		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+		if (plane != &priv->f0) {
 			xy_reg = JZ_REG_LCD_XYP1;
 			size_reg = JZ_REG_LCD_SIZE1;
 		} else {
@@ -561,7 +560,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		height = newstate->src_h >> 16;
 		cpp = newstate->fb->format->cpp[0];
 
-		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
+		if (!priv->soc_info->has_osd || plane == &priv->f0)
 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
 		else
 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 5ae6adab8306..3b1091e7c0cd 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -767,7 +767,7 @@ static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
 
 	err = drm_universal_plane_init(drm, plane, 1, &ingenic_ipu_plane_funcs,
 				       soc_info->formats, soc_info->num_formats,
-				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+				       NULL, DRM_PLANE_TYPE_OVERLAY, NULL);
 	if (err) {
 		dev_err(dev, "Failed to init plane: %i\n", err);
 		return err;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified
  2021-03-29 17:50 [PATCH 0/2] drm/ingenic: IPU plane fixes Paul Cercueil
  2021-03-29 17:50 ` [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY Paul Cercueil
@ 2021-03-29 17:50 ` Paul Cercueil
  2021-04-12 14:32   ` Paul Cercueil
  2021-04-15  9:26   ` Maxime Ripard
  1 sibling, 2 replies; 8+ messages in thread
From: Paul Cercueil @ 2021-03-29 17:50 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Simon Ser
  Cc: Sam Ravnborg, od, linux-mips, dri-devel, linux-kernel,
	Paul Cercueil, stable

Avoid requesting a full modeset if the sharpness property is not
modified, because then we don't actually need it.

Fixes: fc1acf317b01 ("drm/ingenic: Add support for the IPU")
Cc: <stable@vger.kernel.org> # 5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-ipu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 3b1091e7c0cd..95b665c4a7b0 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -640,10 +640,12 @@ ingenic_ipu_plane_atomic_set_property(struct drm_plane *plane,
 {
 	struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane);
 	struct drm_crtc_state *crtc_state;
+	bool mode_changed;
 
 	if (property != ipu->sharpness_prop)
 		return -EINVAL;
 
+	mode_changed = val != ipu->sharpness;
 	ipu->sharpness = val;
 
 	if (state->crtc) {
@@ -651,7 +653,7 @@ ingenic_ipu_plane_atomic_set_property(struct drm_plane *plane,
 		if (WARN_ON(!crtc_state))
 			return -EINVAL;
 
-		crtc_state->mode_changed = true;
+		crtc_state->mode_changed |= mode_changed;
 	}
 
 	return 0;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY
  2021-03-29 17:50 ` [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY Paul Cercueil
@ 2021-03-30  7:23   ` Simon Ser
  2021-03-30 11:53     ` Paul Cercueil
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Ser @ 2021-03-30  7:23 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Sam Ravnborg, od, linux-mips, dri-devel,
	linux-kernel, stable

> It should have been an OVERLAY from the beginning. The documentation
> stipulates that there should be an unique PRIMARY plane per CRTC.

Thanks for the quick patch! One comment below…

> Fixes: fc1acf317b01 ("drm/ingenic: Add support for the IPU")
> Cc: <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++------
>  drivers/gpu/drm/ingenic/ingenic-ipu.c     |  2 +-
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 29742ec5ab95..09225b770bb8 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -419,7 +419,7 @@ static void ingenic_drm_plane_enable(struct ingenic_drm *priv,
>  	unsigned int en_bit;
>
>  	if (priv->soc_info->has_osd) {
> -		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> +		if (plane != &priv->f0)

I don't know about this driver but… is this really the same as the previous
condition? The previous condition would match two planes, this one seems to
match only a single plane. What am I missing?


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY
  2021-03-30  7:23   ` Simon Ser
@ 2021-03-30 11:53     ` Paul Cercueil
  2021-03-30 12:59       ` Simon Ser
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2021-03-30 11:53 UTC (permalink / raw)
  To: Simon Ser
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Sam Ravnborg, od, linux-mips, dri-devel,
	linux-kernel, stable

Hi Simon,

Le mar. 30 mars 2021 à 7:23, Simon Ser <contact@emersion.fr> a écrit :
>>  It should have been an OVERLAY from the beginning. The documentation
>>  stipulates that there should be an unique PRIMARY plane per CRTC.
> 
> Thanks for the quick patch! One comment below…
> 
>>  Fixes: fc1acf317b01 ("drm/ingenic: Add support for the IPU")
>>  Cc: <stable@vger.kernel.org> # 5.8+
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++------
>>   drivers/gpu/drm/ingenic/ingenic-ipu.c     |  2 +-
>>   2 files changed, 6 insertions(+), 7 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index 29742ec5ab95..09225b770bb8 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -419,7 +419,7 @@ static void ingenic_drm_plane_enable(struct 
>> ingenic_drm *priv,
>>   	unsigned int en_bit;
>> 
>>   	if (priv->soc_info->has_osd) {
>>  -		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>  +		if (plane != &priv->f0)
> 
> I don't know about this driver but… is this really the same as the 
> previous
> condition? The previous condition would match two planes, this one 
> seems to
> match only a single plane. What am I missing?

There are three planes, which we will call here f0, f1, and ipu.

Previously, the "plane->type == DRM_PLANE_TYPE_PRIMARY" matched f1 and 
ipu. Since ipu is now OVERLAY we have to change the condition or the 
behaviour will be different, as otherwise it would only match the f1 
plane.

Cheers,
-Paul



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY
  2021-03-30 11:53     ` Paul Cercueil
@ 2021-03-30 12:59       ` Simon Ser
  0 siblings, 0 replies; 8+ messages in thread
From: Simon Ser @ 2021-03-30 12:59 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Sam Ravnborg, od, linux-mips, dri-devel,
	linux-kernel, stable

On Tuesday, March 30th, 2021 at 1:53 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> > I don't know about this driver but… is this really the same as the
> > previous
> > condition? The previous condition would match two planes, this one
> > seems to
> > match only a single plane. What am I missing?
>
> There are three planes, which we will call here f0, f1, and ipu.
> Previously, the "plane->type == DRM_PLANE_TYPE_PRIMARY" matched f1 and
> ipu. Since ipu is now OVERLAY we have to change the condition or the
> behaviour will be different, as otherwise it would only match the f1
> plane.

Oh okay, I thought f0 was one of the primary planes, but it's not.
Thanks for the explanation.

For the user-space visible change:

Acked-by: Simon Ser <contact@emersion.fr>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified
  2021-03-29 17:50 ` [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified Paul Cercueil
@ 2021-04-12 14:32   ` Paul Cercueil
  2021-04-15  9:26   ` Maxime Ripard
  1 sibling, 0 replies; 8+ messages in thread
From: Paul Cercueil @ 2021-04-12 14:32 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Simon Ser
  Cc: Sam Ravnborg, od, linux-mips, dri-devel, linux-kernel, stable

Can I have an ACK for this patch?

Cheers,
-Paul

Le lun. 29 mars 2021 à 18:50, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> Avoid requesting a full modeset if the sharpness property is not
> modified, because then we don't actually need it.
> 
> Fixes: fc1acf317b01 ("drm/ingenic: Add support for the IPU")
> Cc: <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-ipu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c 
> b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> index 3b1091e7c0cd..95b665c4a7b0 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> @@ -640,10 +640,12 @@ ingenic_ipu_plane_atomic_set_property(struct 
> drm_plane *plane,
>  {
>  	struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane);
>  	struct drm_crtc_state *crtc_state;
> +	bool mode_changed;
> 
>  	if (property != ipu->sharpness_prop)
>  		return -EINVAL;
> 
> +	mode_changed = val != ipu->sharpness;
>  	ipu->sharpness = val;
> 
>  	if (state->crtc) {
> @@ -651,7 +653,7 @@ ingenic_ipu_plane_atomic_set_property(struct 
> drm_plane *plane,
>  		if (WARN_ON(!crtc_state))
>  			return -EINVAL;
> 
> -		crtc_state->mode_changed = true;
> +		crtc_state->mode_changed |= mode_changed;
>  	}
> 
>  	return 0;
> --
> 2.30.2
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified
  2021-03-29 17:50 ` [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified Paul Cercueil
  2021-04-12 14:32   ` Paul Cercueil
@ 2021-04-15  9:26   ` Maxime Ripard
  1 sibling, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2021-04-15  9:26 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: David Airlie, Daniel Vetter, Maarten Lankhorst,
	Thomas Zimmermann, Simon Ser, Sam Ravnborg, od, linux-mips,
	dri-devel, linux-kernel, stable

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

Hi,

On Mon, Mar 29, 2021 at 06:50:46PM +0100, Paul Cercueil wrote:
> Avoid requesting a full modeset if the sharpness property is not
> modified, because then we don't actually need it.
> 
> Fixes: fc1acf317b01 ("drm/ingenic: Add support for the IPU")
> Cc: <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-ipu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> index 3b1091e7c0cd..95b665c4a7b0 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> @@ -640,10 +640,12 @@ ingenic_ipu_plane_atomic_set_property(struct drm_plane *plane,
>  {
>  	struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane);
>  	struct drm_crtc_state *crtc_state;
> +	bool mode_changed;
>  
>  	if (property != ipu->sharpness_prop)
>  		return -EINVAL;
>  
> +	mode_changed = val != ipu->sharpness;
>  	ipu->sharpness = val;
>  
>  	if (state->crtc) {
> @@ -651,7 +653,7 @@ ingenic_ipu_plane_atomic_set_property(struct drm_plane *plane,
>  		if (WARN_ON(!crtc_state))
>  			return -EINVAL;
>  
> -		crtc_state->mode_changed = true;
> +		crtc_state->mode_changed |= mode_changed;
>  	}

I'd just change the condition from

if (state->crtc)

to

if (state->crtc && val != ipu->sharpness)

It's going to be easier to extend if you ever need to. Also, drivers
usually do this in atomic_check, is there a specific reason to do it in
atomic_set_property?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-04-15  9:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29 17:50 [PATCH 0/2] drm/ingenic: IPU plane fixes Paul Cercueil
2021-03-29 17:50 ` [PATCH 1/2] drm/ingenic: Switch IPU plane to type OVERLAY Paul Cercueil
2021-03-30  7:23   ` Simon Ser
2021-03-30 11:53     ` Paul Cercueil
2021-03-30 12:59       ` Simon Ser
2021-03-29 17:50 ` [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified Paul Cercueil
2021-04-12 14:32   ` Paul Cercueil
2021-04-15  9:26   ` Maxime Ripard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).