Re: [PATCH v2 1/4] drm/sun4i: Reimplement plane z position setting logic
Hi!
Dne nedelja, 29. december 2019 ob 17:28:25 CET je
roman.stratiie...@globallogic.com napisal(a):
> From: Roman Stratiienko
>
> To set blending channel order register software needs to know state and
> position of each channel, which impossible at plane commit stage.
>
> Move this procedure to atomic_flush stage, where all necessary information
> is available.
>
> Fixes: f88c5ee77496 ("drm/sun4i: Implement zpos for DE2")
> Signed-off-by: Roman Stratiienko
> ---
> v2:
> - Use SUN8I_MIXER_MAX_LAYERS macro
> - Use plane_cnt instead of hard-coded number
> - Put initialization of channel_zpos into for loop
> - Add 'Fixes' line to the commit message
> - Minor clean-ups
> - Comments added
> ---
> drivers/gpu/drm/sun4i/sun8i_mixer.c| 52 +-
> drivers/gpu/drm/sun4i/sun8i_mixer.h| 5 +++
> drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 +++--
> drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++
> 4 files changed, 66 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 8b803eb903b8..d306ad5dc093
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -259,8 +259,54 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32
> format)
>
> static void sun8i_mixer_commit(struct sunxi_engine *engine)
> {
> - DRM_DEBUG_DRIVER("Committing changes\n");
> + struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> + u32 base = sun8i_blender_base(mixer);
> + int i, j = 0;
> + int channel_by_zpos[SUN8I_MIXER_MAX_LAYERS];
> + u32 route = 0, pipe_ctl = 0;
> + int plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
Can you please sort above lines by size (longest to shortest) as much as
possible?
> +
> + DRM_DEBUG_DRIVER("Update blender routing\n");
> + /* Assume that values in mixer->channel_zpos[] are unique except -1
*/
I would remove "Assume that" because it's guaranteed by zpos normalization in
DRM core.
> +
> + for (i = 0; i < plane_cnt; i++)
> + channel_by_zpos[i] = -1;
>
> + /* Sort by zpos */
> + for (i = 0; i < plane_cnt; i++) {
> + int zpos = mixer->channel_zpos[i];
> +
> + if (zpos >= 0 && zpos < plane_cnt)
> + channel_by_zpos[zpos] = i;
> + }
> +
> + /* Route enabled blending channels first */
> + for (i = 0; i < plane_cnt; i++) {
> + int ch = channel_by_zpos[i];
> +
> + if (ch >= 0) {
> + pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> + route |= ch <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> + j++;
> + }
> + }
> +
> + /* Set remaining routing fields to match disabled channel indices */
> + for (i = 0; i < SUN8I_MIXER_MAX_LAYERS && j <
SUN8I_MIXER_MAX_LAYERS;
> + i++) {
> + if (mixer->channel_zpos[i] < 0) {
> + route |= i <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> + j++;
> + }
> + }
Is above for loop really necessary? If pipe is not enabled I don't think you
need to set a route.
> +
> + regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_PIPE_CTL(base),
> +SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK,
pipe_ctl);
> +
> + regmap_write(mixer->engine.regs,
> + SUN8I_MIXER_BLEND_ROUTE(base), route);
> +
> + DRM_DEBUG_DRIVER("Committing changes\n");
> regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
>SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> }
> @@ -485,10 +531,12 @@ static int sun8i_mixer_bind(struct device *dev, struct
> device *master, SUN8I_MIXER_BLEND_COLOR_BLACK);
>
> plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> - for (i = 0; i < plane_cnt; i++)
> + for (i = 0; i < plane_cnt; i++) {
> + mixer->channel_zpos[i] = -1;
> regmap_write(mixer->engine.regs,
>SUN8I_MIXER_BLEND_MODE(base, i),
>SUN8I_MIXER_BLEND_MODE_DEF);
> + }
>
> regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_PIPE_CTL(base),
> SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h index c6cc94057faf..b193d9d1db66
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -13,6 +13,8 @@
> #include "sun8i_csc.h"
> #include "sunxi_engine.h"
>
> +#define SUN8I_MIXER_MAX_LAYERS 5
> +
> #define SUN8I_MIXER_SIZE(w, h) (((h) - 1) << 16 |
((w) - 1))
> #define SUN8I_MIXER_COORD(x, y) ((y) << 16 | (x))
>
> @@ -176,6 +178,9 @@ struct sun8i_mixer {
>
> struct clk *bus_clk;
> struct clk *mod_clk;
> +
Re: [PATCH v2 1/4] drm/sun4i: Reimplement plane z position setting logic
Please HOLD this until v3.
I forgot about blender channel frame coords that updated according to
zpos and can have gap between channels.
I will also try to simplify this patch.
On Sun, Dec 29, 2019 at 6:28 PM wrote:
>
> From: Roman Stratiienko
>
> To set blending channel order register software needs to know state and
> position of each channel, which impossible at plane commit stage.
>
> Move this procedure to atomic_flush stage, where all necessary information
> is available.
>
> Fixes: f88c5ee77496 ("drm/sun4i: Implement zpos for DE2")
> Signed-off-by: Roman Stratiienko
> ---
> v2:
> - Use SUN8I_MIXER_MAX_LAYERS macro
> - Use plane_cnt instead of hard-coded number
> - Put initialization of channel_zpos into for loop
> - Add 'Fixes' line to the commit message
> - Minor clean-ups
> - Comments added
> ---
> drivers/gpu/drm/sun4i/sun8i_mixer.c| 52 +-
> drivers/gpu/drm/sun4i/sun8i_mixer.h| 5 +++
> drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 +++--
> drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++
> 4 files changed, 66 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 8b803eb903b8..d306ad5dc093 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -259,8 +259,54 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32
> format)
>
> static void sun8i_mixer_commit(struct sunxi_engine *engine)
> {
> - DRM_DEBUG_DRIVER("Committing changes\n");
> + struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> + u32 base = sun8i_blender_base(mixer);
> + int i, j = 0;
> + int channel_by_zpos[SUN8I_MIXER_MAX_LAYERS];
> + u32 route = 0, pipe_ctl = 0;
> + int plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> +
> + DRM_DEBUG_DRIVER("Update blender routing\n");
> + /* Assume that values in mixer->channel_zpos[] are unique except -1 */
> +
> + for (i = 0; i < plane_cnt; i++)
> + channel_by_zpos[i] = -1;
>
> + /* Sort by zpos */
> + for (i = 0; i < plane_cnt; i++) {
> + int zpos = mixer->channel_zpos[i];
> +
> + if (zpos >= 0 && zpos < plane_cnt)
> + channel_by_zpos[zpos] = i;
> + }
> +
> + /* Route enabled blending channels first */
> + for (i = 0; i < plane_cnt; i++) {
> + int ch = channel_by_zpos[i];
> +
> + if (ch >= 0) {
> + pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> + route |= ch << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> + j++;
> + }
> + }
> +
> + /* Set remaining routing fields to match disabled channel indices */
> + for (i = 0; i < SUN8I_MIXER_MAX_LAYERS && j < SUN8I_MIXER_MAX_LAYERS;
> +i++) {
> + if (mixer->channel_zpos[i] < 0) {
> + route |= i << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> + j++;
> + }
> + }
> +
> + regmap_update_bits(mixer->engine.regs,
> SUN8I_MIXER_BLEND_PIPE_CTL(base),
> + SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, pipe_ctl);
> +
> + regmap_write(mixer->engine.regs,
> +SUN8I_MIXER_BLEND_ROUTE(base), route);
> +
> + DRM_DEBUG_DRIVER("Committing changes\n");
> regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> }
> @@ -485,10 +531,12 @@ static int sun8i_mixer_bind(struct device *dev, struct
> device *master,
> SUN8I_MIXER_BLEND_COLOR_BLACK);
>
> plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> - for (i = 0; i < plane_cnt; i++)
> + for (i = 0; i < plane_cnt; i++) {
> + mixer->channel_zpos[i] = -1;
> regmap_write(mixer->engine.regs,
> SUN8I_MIXER_BLEND_MODE(base, i),
> SUN8I_MIXER_BLEND_MODE_DEF);
> + }
>
> regmap_update_bits(mixer->engine.regs,
> SUN8I_MIXER_BLEND_PIPE_CTL(base),
>SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> index c6cc94057faf..b193d9d1db66 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -13,6 +13,8 @@
> #include "sun8i_csc.h"
> #include "sunxi_engine.h"
>
> +#define SUN8I_MIXER_MAX_LAYERS 5
> +
> #define SUN8I_MIXER_SIZE(w, h) (((h) - 1) << 16 | ((w) - 1))
> #define SUN8I_MIXER_COORD(x, y)((y) << 16 | (x))
>
> @@ -176,6 +178,9 @@ struct sun8i_mixer {
>
> struct clk *bus_clk;
> struct clk *mod_clk;
> +
> + /* -1 means that layer is disabled */
>
Re: [PATCH v2 1/4] drm/sun4i: Reimplement plane z position setting logic
False alarm. Since DRM sets normalized ZPOS there should be no gaps
and all should work as expected.
On Mon, Dec 30, 2019 at 1:25 PM Roman Stratiienko
wrote:
>
> Please HOLD this until v3.
> I forgot about blender channel frame coords that updated according to
> zpos and can have gap between channels.
> I will also try to simplify this patch.
>
> On Sun, Dec 29, 2019 at 6:28 PM wrote:
> >
> > From: Roman Stratiienko
> >
> > To set blending channel order register software needs to know state and
> > position of each channel, which impossible at plane commit stage.
> >
> > Move this procedure to atomic_flush stage, where all necessary information
> > is available.
> >
> > Fixes: f88c5ee77496 ("drm/sun4i: Implement zpos for DE2")
> > Signed-off-by: Roman Stratiienko
> > ---
> > v2:
> > - Use SUN8I_MIXER_MAX_LAYERS macro
> > - Use plane_cnt instead of hard-coded number
> > - Put initialization of channel_zpos into for loop
> > - Add 'Fixes' line to the commit message
> > - Minor clean-ups
> > - Comments added
> > ---
> > drivers/gpu/drm/sun4i/sun8i_mixer.c| 52 +-
> > drivers/gpu/drm/sun4i/sun8i_mixer.h| 5 +++
> > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 +++--
> > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++
> > 4 files changed, 66 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > index 8b803eb903b8..d306ad5dc093 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -259,8 +259,54 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32
> > format)
> >
> > static void sun8i_mixer_commit(struct sunxi_engine *engine)
> > {
> > - DRM_DEBUG_DRIVER("Committing changes\n");
> > + struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
> > + u32 base = sun8i_blender_base(mixer);
> > + int i, j = 0;
> > + int channel_by_zpos[SUN8I_MIXER_MAX_LAYERS];
> > + u32 route = 0, pipe_ctl = 0;
> > + int plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> > +
> > + DRM_DEBUG_DRIVER("Update blender routing\n");
> > + /* Assume that values in mixer->channel_zpos[] are unique except -1
> > */
> > +
> > + for (i = 0; i < plane_cnt; i++)
> > + channel_by_zpos[i] = -1;
> >
> > + /* Sort by zpos */
> > + for (i = 0; i < plane_cnt; i++) {
> > + int zpos = mixer->channel_zpos[i];
> > +
> > + if (zpos >= 0 && zpos < plane_cnt)
> > + channel_by_zpos[zpos] = i;
> > + }
> > +
> > + /* Route enabled blending channels first */
> > + for (i = 0; i < plane_cnt; i++) {
> > + int ch = channel_by_zpos[i];
> > +
> > + if (ch >= 0) {
> > + pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
> > + route |= ch <<
> > SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > + j++;
> > + }
> > + }
> > +
> > + /* Set remaining routing fields to match disabled channel indices */
> > + for (i = 0; i < SUN8I_MIXER_MAX_LAYERS && j <
> > SUN8I_MIXER_MAX_LAYERS;
> > +i++) {
> > + if (mixer->channel_zpos[i] < 0) {
> > + route |= i << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
> > + j++;
> > + }
> > + }
> > +
> > + regmap_update_bits(mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> > + SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, pipe_ctl);
> > +
> > + regmap_write(mixer->engine.regs,
> > +SUN8I_MIXER_BLEND_ROUTE(base), route);
> > +
> > + DRM_DEBUG_DRIVER("Committing changes\n");
> > regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
> > SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
> > }
> > @@ -485,10 +531,12 @@ static int sun8i_mixer_bind(struct device *dev,
> > struct device *master,
> > SUN8I_MIXER_BLEND_COLOR_BLACK);
> >
> > plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
> > - for (i = 0; i < plane_cnt; i++)
> > + for (i = 0; i < plane_cnt; i++) {
> > + mixer->channel_zpos[i] = -1;
> > regmap_write(mixer->engine.regs,
> > SUN8I_MIXER_BLEND_MODE(base, i),
> > SUN8I_MIXER_BLEND_MODE_DEF);
> > + }
> >
> > regmap_update_bits(mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PIPE_CTL(base),
> >SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > index c6cc94057faf..b193d9d1db66 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -13,6 +13,8 @@
> > #include "sun8i_csc.h"
> > #include "sunxi_engine.h"
> >
>
[PATCH v2 1/4] drm/sun4i: Reimplement plane z position setting logic
From: Roman Stratiienko
To set blending channel order register software needs to know state and
position of each channel, which impossible at plane commit stage.
Move this procedure to atomic_flush stage, where all necessary information
is available.
Fixes: f88c5ee77496 ("drm/sun4i: Implement zpos for DE2")
Signed-off-by: Roman Stratiienko
---
v2:
- Use SUN8I_MIXER_MAX_LAYERS macro
- Use plane_cnt instead of hard-coded number
- Put initialization of channel_zpos into for loop
- Add 'Fixes' line to the commit message
- Minor clean-ups
- Comments added
---
drivers/gpu/drm/sun4i/sun8i_mixer.c| 52 +-
drivers/gpu/drm/sun4i/sun8i_mixer.h| 5 +++
drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 +++--
drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++
4 files changed, 66 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index 8b803eb903b8..d306ad5dc093 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -259,8 +259,54 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32
format)
static void sun8i_mixer_commit(struct sunxi_engine *engine)
{
- DRM_DEBUG_DRIVER("Committing changes\n");
+ struct sun8i_mixer *mixer = engine_to_sun8i_mixer(engine);
+ u32 base = sun8i_blender_base(mixer);
+ int i, j = 0;
+ int channel_by_zpos[SUN8I_MIXER_MAX_LAYERS];
+ u32 route = 0, pipe_ctl = 0;
+ int plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
+
+ DRM_DEBUG_DRIVER("Update blender routing\n");
+ /* Assume that values in mixer->channel_zpos[] are unique except -1 */
+
+ for (i = 0; i < plane_cnt; i++)
+ channel_by_zpos[i] = -1;
+ /* Sort by zpos */
+ for (i = 0; i < plane_cnt; i++) {
+ int zpos = mixer->channel_zpos[i];
+
+ if (zpos >= 0 && zpos < plane_cnt)
+ channel_by_zpos[zpos] = i;
+ }
+
+ /* Route enabled blending channels first */
+ for (i = 0; i < plane_cnt; i++) {
+ int ch = channel_by_zpos[i];
+
+ if (ch >= 0) {
+ pipe_ctl |= SUN8I_MIXER_BLEND_PIPE_CTL_EN(j);
+ route |= ch << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
+ j++;
+ }
+ }
+
+ /* Set remaining routing fields to match disabled channel indices */
+ for (i = 0; i < SUN8I_MIXER_MAX_LAYERS && j < SUN8I_MIXER_MAX_LAYERS;
+i++) {
+ if (mixer->channel_zpos[i] < 0) {
+ route |= i << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(j);
+ j++;
+ }
+ }
+
+ regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
+ SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, pipe_ctl);
+
+ regmap_write(mixer->engine.regs,
+SUN8I_MIXER_BLEND_ROUTE(base), route);
+
+ DRM_DEBUG_DRIVER("Committing changes\n");
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF,
SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
}
@@ -485,10 +531,12 @@ static int sun8i_mixer_bind(struct device *dev, struct
device *master,
SUN8I_MIXER_BLEND_COLOR_BLACK);
plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
- for (i = 0; i < plane_cnt; i++)
+ for (i = 0; i < plane_cnt; i++) {
+ mixer->channel_zpos[i] = -1;
regmap_write(mixer->engine.regs,
SUN8I_MIXER_BLEND_MODE(base, i),
SUN8I_MIXER_BLEND_MODE_DEF);
+ }
regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL(base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index c6cc94057faf..b193d9d1db66 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -13,6 +13,8 @@
#include "sun8i_csc.h"
#include "sunxi_engine.h"
+#define SUN8I_MIXER_MAX_LAYERS 5
+
#define SUN8I_MIXER_SIZE(w, h) (((h) - 1) << 16 | ((w) - 1))
#define SUN8I_MIXER_COORD(x, y)((y) << 16 | (x))
@@ -176,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk;
struct clk *mod_clk;
+
+ /* -1 means that layer is disabled */
+ int channel_zpos[SUN8I_MIXER_MAX_LAYERS];
};
static inline struct sun8i_mixer *
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index c87fd842918e..ee7c13d8710f 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -24,12 +24,10 @@
#include "sun8i_ui_scaler.h"
static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
-
