[PATCH 1/2] drm/sun4i: sun4i: Register quirks with the backend structure

[PATCH 1/2] drm/sun4i: sun4i: Register quirks with the backend structure

[Paul Kocialkowski]

In prevision for introducing a new quirk that will be used at atomic
plane check time, register the quirks structure with the backend
structure. This way, it can easily be grabbed where needed.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c | 2 ++
 drivers/gpu/drm/sun4i/sun4i_backend.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 17dca4df00f2..a3cc398d4d80 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -909,6 +909,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
 				    : SUN4I_BACKEND_MODCTL_OUT_LCD0));
 	}
 
+	backend->quirks = quirks;
+
 	return 0;
 
 err_disable_ram_clk:
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h
index e33be2307c67..71de6147b483 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.h
@@ -187,6 +187,8 @@ struct sun4i_backend {
 	/* Protects against races in the frontend teardown */
 	spinlock_t		frontend_lock;
 	bool			frontend_teardown;
+
+	const struct sun4i_backend_quirks	*quirks;
 };
 
 static inline struct sun4i_backend *
-- 
2.17.1

[PATCH 2/2] drm/sun4i: sun4i: Introduce a quirk for lowest plane alpha support

[Paul Kocialkowski]

Not all sunxi platforms with the first version of the Display Engine
support an alpha component on the plane with the lowest z position
(as in: lowest z-pos), that gets blended with the background color.

In particular, the A13 is known to have this limitation. However, it was
recently discovered that the A20 and A33 are capable of having alpha on
their lowest plane.

Thus, this introduces a specific quirk to indicate such support,
per-platform. Since this was not tested on sun4i and sun6i platforms, a
conservative approach is kept and this feature is not supported.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index a3cc398d4d80..cdc4a8a91ea2 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -35,6 +35,8 @@
 struct sun4i_backend_quirks {
 	/* backend <-> TCON muxing selection done in backend */
 	bool needs_output_muxing;
+	/* alpha at the lowest z position is not always supported */
+	bool supports_lowest_plane_alpha;
 };
 
 static void sun4i_backend_apply_color_correction(struct sunxi_engine *engine)
@@ -484,6 +486,7 @@ static void sun4i_backend_atomic_begin(struct sunxi_engine *engine,
 static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 				      struct drm_crtc_state *crtc_state)
 {
+	struct sun4i_backend *backend = engine_to_sun4i_backend(engine);
 	struct drm_plane_state *plane_states[SUN4I_BACKEND_NUM_LAYERS] = { 0 };
 	struct drm_atomic_state *state = crtc_state->state;
 	struct drm_device *drm = state->dev;
@@ -584,8 +587,9 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 	}
 
 	/* We can't have an alpha plane at the lowest position */
-	if (plane_states[0]->fb->format->has_alpha ||
-	    (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE))
+	if ((plane_states[0]->fb->format->has_alpha ||
+	    (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE)) &&
+	    !backend->quirks->supports_lowest_plane_alpha)
 		return -EINVAL;
 
 	for (i = 1; i < num_planes; i++) {
@@ -970,9 +974,11 @@ static const struct sun4i_backend_quirks sun6i_backend_quirks = {
 
 static const struct sun4i_backend_quirks sun7i_backend_quirks = {
 	.needs_output_muxing = true,
+	.supports_lowest_plane_alpha = true,
 };
 
 static const struct sun4i_backend_quirks sun8i_a33_backend_quirks = {
+	.supports_lowest_plane_alpha = true,
 };
 
 static const struct sun4i_backend_quirks sun9i_backend_quirks = {
-- 
2.17.1

Re: [PATCH 2/2] drm/sun4i: sun4i: Introduce a quirk for lowest plane alpha support

[Maxime Ripard]

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

On Tue, Jul 17, 2018 at 10:52:30AM +0200, Paul Kocialkowski wrote:
> Not all sunxi platforms with the first version of the Display Engine
> support an alpha component on the plane with the lowest z position
> (as in: lowest z-pos), that gets blended with the background color.
> 
> In particular, the A13 is known to have this limitation. However, it was
> recently discovered that the A20 and A33 are capable of having alpha on
> their lowest plane.
> 
> Thus, this introduces a specific quirk to indicate such support,
> per-platform. Since this was not tested on sun4i and sun6i platforms, a
> conservative approach is kept and this feature is not supported.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_backend.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index a3cc398d4d80..cdc4a8a91ea2 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -35,6 +35,8 @@
>  struct sun4i_backend_quirks {
>  	/* backend <-> TCON muxing selection done in backend */
>  	bool needs_output_muxing;
> +	/* alpha at the lowest z position is not always supported */
> +	bool supports_lowest_plane_alpha;
>  };
>  
>  static void sun4i_backend_apply_color_correction(struct sunxi_engine *engine)
> @@ -484,6 +486,7 @@ static void sun4i_backend_atomic_begin(struct sunxi_engine *engine,
>  static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
>  				      struct drm_crtc_state *crtc_state)
>  {
> +	struct sun4i_backend *backend = engine_to_sun4i_backend(engine);
>  	struct drm_plane_state *plane_states[SUN4I_BACKEND_NUM_LAYERS] = { 0 };

Your new variable should be here.

>  	struct drm_atomic_state *state = crtc_state->state;
>  	struct drm_device *drm = state->dev;
> @@ -584,8 +587,9 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
>  	}
>  
>  	/* We can't have an alpha plane at the lowest position */
> -	if (plane_states[0]->fb->format->has_alpha ||
> -	    (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE))
> +	if ((plane_states[0]->fb->format->has_alpha ||
> +	    (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE)) &&
> +	    !backend->quirks->supports_lowest_plane_alpha)
>  		return -EINVAL;

This only partially does the job. This only allows to have an alpha
plane at the lowest position, but the fact that the alpha works at the
lowest position also means you can have two alpha planes now, and you
didn't change that check.

The pipe allocation algorithm would also need to be checked.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH 2/2] drm/sun4i: sun4i: Introduce a quirk for lowest plane alpha support

[Paul Kocialkowski]

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

Hi,

On Tue, 2018-07-17 at 14:25 +0200, Maxime Ripard wrote:
> On Tue, Jul 17, 2018 at 10:52:30AM +0200, Paul Kocialkowski wrote:
> > Not all sunxi platforms with the first version of the Display Engine
> > support an alpha component on the plane with the lowest z position
> > (as in: lowest z-pos), that gets blended with the background color.
> > 
> > In particular, the A13 is known to have this limitation. However, it was
> > recently discovered that the A20 and A33 are capable of having alpha on
> > their lowest plane.
> > 
> > Thus, this introduces a specific quirk to indicate such support,
> > per-platform. Since this was not tested on sun4i and sun6i platforms, a
> > conservative approach is kept and this feature is not supported.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_backend.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index a3cc398d4d80..cdc4a8a91ea2 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -35,6 +35,8 @@
> >  struct sun4i_backend_quirks {
> >  	/* backend <-> TCON muxing selection done in backend */
> >  	bool needs_output_muxing;
> > +	/* alpha at the lowest z position is not always supported */
> > +	bool supports_lowest_plane_alpha;
> >  };
> >  
> >  static void sun4i_backend_apply_color_correction(struct sunxi_engine *engine)
> > @@ -484,6 +486,7 @@ static void sun4i_backend_atomic_begin(struct sunxi_engine *engine,
> >  static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> >  				      struct drm_crtc_state *crtc_state)
> >  {
> > +	struct sun4i_backend *backend = engine_to_sun4i_backend(engine);
> >  	struct drm_plane_state *plane_states[SUN4I_BACKEND_NUM_LAYERS] = { 0 };
> 
> Your new variable should be here.

Ok, will do in v2.

> >  	struct drm_atomic_state *state = crtc_state->state;
> >  	struct drm_device *drm = state->dev;
> > @@ -584,8 +587,9 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> >  	}
> >  
> >  	/* We can't have an alpha plane at the lowest position */
> > -	if (plane_states[0]->fb->format->has_alpha ||
> > -	    (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE))
> > +	if ((plane_states[0]->fb->format->has_alpha ||
> > +	    (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE)) &&
> > +	    !backend->quirks->supports_lowest_plane_alpha)
> >  		return -EINVAL;
> 
> This only partially does the job. This only allows to have an alpha
> plane at the lowest position, but the fact that the alpha works at the
> lowest position also means you can have two alpha planes now, and you
> didn't change that check.

You're right, the number of available planes with alpha has to be
changed accordingly. Will do in v2.

> The pipe allocation algorithm would also need to be checked.

From my understanding, the limitation on the number of alpha planes only
takes effect after pipe allocation, so this change doesn't require
modifying the allocation algorithm.

Cheers and thanks for the review,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [linux-sunxi] [PATCH 2/2] drm/sun4i: sun4i: Introduce a quirk for lowest plane alpha support

[Julian Calaby]

Hi Paul,

On Tue, Jul 17, 2018 at 6:53 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Not all sunxi platforms with the first version of the Display Engine
> support an alpha component on the plane with the lowest z position
> (as in: lowest z-pos), that gets blended with the background color.
>
> In particular, the A13 is known to have this limitation. However, it was
> recently discovered that the A20 and A33 are capable of having alpha on
> their lowest plane.
>
> Thus, this introduces a specific quirk to indicate such support,
> per-platform. Since this was not tested on sun4i and sun6i platforms, a
> conservative approach is kept and this feature is not supported.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_backend.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index a3cc398d4d80..cdc4a8a91ea2 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -584,8 +587,9 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
>         }
>
>         /* We can't have an alpha plane at the lowest position */
> -       if (plane_states[0]->fb->format->has_alpha ||
> -           (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE))
> +       if ((plane_states[0]->fb->format->has_alpha ||
> +           (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE)) &&
> +           !backend->quirks->supports_lowest_plane_alpha)

From a readability perspective, it'd be fractionally nicer if the
quirk check was before the alpha checks.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 2/2] drm/sun4i: sun4i: Introduce a quirk for lowest plane alpha support

[Paul Kocialkowski]

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

Hi,

On Tue, 2018-07-17 at 22:41 +1000, Julian Calaby wrote:
> Hi Paul,
> 
> On Tue, Jul 17, 2018 at 6:53 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > 
> > Not all sunxi platforms with the first version of the Display Engine
> > support an alpha component on the plane with the lowest z position
> > (as in: lowest z-pos), that gets blended with the background color.
> > 
> > In particular, the A13 is known to have this limitation. However, it was
> > recently discovered that the A20 and A33 are capable of having alpha on
> > their lowest plane.
> > 
> > Thus, this introduces a specific quirk to indicate such support,
> > per-platform. Since this was not tested on sun4i and sun6i platforms, a
> > conservative approach is kept and this feature is not supported.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_backend.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index a3cc398d4d80..cdc4a8a91ea2 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -584,8 +587,9 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
> >         }
> > 
> >         /* We can't have an alpha plane at the lowest position */
> > -       if (plane_states[0]->fb->format->has_alpha ||
> > -           (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE))
> > +       if ((plane_states[0]->fb->format->has_alpha ||
> > +           (plane_states[0]->alpha != DRM_BLEND_ALPHA_OPAQUE)) &&
> > +           !backend->quirks->supports_lowest_plane_alpha)
> 
> From a readability perspective, it'd be fractionally nicer if the
> quirk check was before the alpha checks.

Agreed, I will do that in v2.

Thanks!

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]