From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com --- drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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; + int channel_by_zpos[4] = {-1, -1, -1, -1}; + u32 route = 0, pipe_ctl = 0; + + DRM_DEBUG_DRIVER("Update blender routing\n"); + for (i = 0; i < 4; i++) { + int zpos = mixer->channel_zpos[i]; + + if (zpos >= 0 && zpos < 4) + channel_by_zpos[zpos] = i; + } + + j = 0; + for (i = 0; i < 4; 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++; + } + } + + for (i = 0; i < 4 && j < 4; i++) { + int zpos = mixer->channel_zpos[i];
+ if (zpos < 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); } @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, mixer->engine.ops = &sun8i_engine_ops; mixer->engine.node = dev->of_node;
+ mixer->channel_zpos[0] = -1; + mixer->channel_zpos[1] = -1; + mixer->channel_zpos[2] = -1; + mixer->channel_zpos[3] = -1; + mixer->channel_zpos[4] = -1; + /* * While this function can fail, we shouldn't do anything * if this happens. Some early DE2 DT entries don't provide diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -178,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk; struct clk *mod_clk; + + /* -1 means that layer is disabled */ + int channel_zpos[5]; };
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 893076716070..23c2f4b68c89 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, - int overlay, bool enable, unsigned int zpos, - unsigned int old_zpos) + int overlay, bool enable, unsigned int zpos) { - u32 val, bld_base, ch_base; + u32 val, ch_base;
- bld_base = sun8i_blender_base(mixer); ch_base = sun8i_channel_base(mixer, channel);
DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n", @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
- if (!enable || zpos != old_zpos) { - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_PIPE_CTL(bld_base), - SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos), - 0); - - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_ROUTE(bld_base), - SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos), - 0); - } - - if (enable) { - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos); - - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_PIPE_CTL(bld_base), - val, val); - - val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos); - - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_ROUTE(bld_base), - SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos), - val); - } + mixer->channel_zpos[channel] = enable ? zpos : -1; }
static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel, @@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state) { struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); - unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
- sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0, - old_zpos); + sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0); }
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, @@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, { struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); unsigned int zpos = plane->state->normalized_zpos; - unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
if (!plane->state->visible) { sun8i_ui_layer_enable(mixer, layer->channel, - layer->overlay, false, 0, old_zpos); + layer->overlay, false, 0); return; }
@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, layer->channel, layer->overlay, plane); sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, - true, zpos, old_zpos); + true, zpos); }
static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = { diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -17,8 +17,7 @@ #include "sun8i_vi_scaler.h"
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, - int overlay, bool enable, unsigned int zpos, - unsigned int old_zpos) + int overlay, bool enable, unsigned int zpos) { u32 val, bld_base, ch_base;
@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
- if (!enable || zpos != old_zpos) { - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_PIPE_CTL(bld_base), - SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos), - 0); - - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_ROUTE(bld_base), - SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos), - 0); - } - - if (enable) { - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos); - - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_PIPE_CTL(bld_base), - val, val); - - val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos); - - regmap_update_bits(mixer->engine.regs, - SUN8I_MIXER_BLEND_ROUTE(bld_base), - SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos), - val); - } + mixer->channel_zpos[channel] = enable ? zpos : -1; }
static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel, @@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state) { struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); - unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
- sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0, - old_zpos); + sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0); }
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, @@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, { struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); unsigned int zpos = plane->state->normalized_zpos; - unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
if (!plane->state->visible) { sun8i_vi_layer_enable(mixer, layer->channel, - layer->overlay, false, 0, old_zpos); + layer->overlay, false, 0); return; }
@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, layer->channel, layer->overlay, plane); sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, - true, zpos, old_zpos); + true, zpos); }
static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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;
- int channel_by_zpos[4] = {-1, -1, -1, -1};
- u32 route = 0, pipe_ctl = 0;
- DRM_DEBUG_DRIVER("Update blender routing\n");
Use drm_dbg().
- for (i = 0; i < 4; i++) {
int zpos = mixer->channel_zpos[i];
channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
if (zpos >= 0 && zpos < 4)channel_by_zpos[zpos] = i;- }
- j = 0;
- for (i = 0; i < 4; 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++;}}
for (i = 0; i < 4 && j < 4; i++) {
int zpos = mixer->channel_zpos[i];if (zpos < 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");
Use drm_dbg().
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF, SUN8I_MIXER_GLOBAL_DBUFF_ENABLE); } @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, mixer->engine.ops = &sun8i_engine_ops; mixer->engine.node = dev->of_node;
- mixer->channel_zpos[0] = -1;
- mixer->channel_zpos[1] = -1;
- mixer->channel_zpos[2] = -1;
- mixer->channel_zpos[3] = -1;
- mixer->channel_zpos[4] = -1;
for loop would be better, especially using proposed macro.
Best regards, Jernej
/* * While this function can fail, we shouldn't do anything * if this happens. Some early DE2 DT entries don't provide diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -178,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk; struct clk *mod_clk;
- /* -1 means that layer is disabled */
- int channel_zpos[5];
};
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 893076716070..23c2f4b68c89 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,
int overlay, bool enable,
unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,
unsigned int zpos)
{
- u32 val, bld_base, ch_base;
- u32 val, ch_base;
bld_base = sun8i_blender_base(mixer); ch_base = sun8i_channel_base(mixer, channel);
DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",
@@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
- if (!enable || zpos != old_zpos) {
regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);- }
- if (enable) {
val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);- }
- mixer->channel_zpos[channel] = enable ? zpos : -1;
}
static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel, @@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state) { struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
false, 0,
old_zpos);
- sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false,
0);
}
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, @@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, { struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
if (!plane->state->visible) { sun8i_ui_layer_enable(mixer, layer->channel,
layer->overlay, false, 0,
old_zpos);
layer->overlay, false, 0);@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, layer->channel, layer->overlay, plane); sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = { diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -17,8 +17,7 @@ #include "sun8i_vi_scaler.h"
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
int overlay, bool enable,
unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,
unsigned int zpos)
{ u32 val, bld_base, ch_base;
@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
- if (!enable || zpos != old_zpos) {
regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);- }
- if (enable) {
val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,
SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);- }
- mixer->channel_zpos[channel] = enable ? zpos : -1;
}
static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel, @@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state) { struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
false, 0,
old_zpos);
- sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false,
0);
}
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, @@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, { struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;
if (!plane->state->visible) { sun8i_vi_layer_enable(mixer, layer->channel,
layer->overlay, false, 0,
old_zpos);
layer->overlay, false, 0);@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, layer->channel, layer->overlay, plane); sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
Hello Jernej,
Thank you for review.
On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec jernej.skrabec@siol.net wrote:
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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;int channel_by_zpos[4] = {-1, -1, -1, -1};u32 route = 0, pipe_ctl = 0;DRM_DEBUG_DRIVER("Update blender routing\n");Use drm_dbg().
for (i = 0; i < 4; i++) {int zpos = mixer->channel_zpos[i];channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
I'll use plane_cnt as it done in mixer_bind
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
Will do.
if (zpos >= 0 && zpos < 4)channel_by_zpos[zpos] = i;}j = 0;for (i = 0; i < 4; 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++;}}for (i = 0; i < 4 && j < 4; i++) {int zpos = mixer->channel_zpos[i];if (zpos < 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");Use drm_dbg().
According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3beb5... , DRM_DEBUG_DRIVER uses drm_dbg. Also, using drm_dbg with category macro would require larger indent, making harder to fit in 80 chars limit. Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF, SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);} @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, mixer->engine.ops = &sun8i_engine_ops; mixer->engine.node = dev->of_node;
mixer->channel_zpos[0] = -1;mixer->channel_zpos[1] = -1;mixer->channel_zpos[2] = -1;mixer->channel_zpos[3] = -1;mixer->channel_zpos[4] = -1;for loop would be better, especially using proposed macro.
I'll put it into already existent for-loop below.
Best regards, Jernej
/* * While this function can fail, we shouldn't do anything * if this happens. Some early DE2 DT entries don't providediff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -178,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk; struct clk *mod_clk;
/* -1 means that layer is disabled */int channel_zpos[5];};
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 893076716070..23c2f4b68c89 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,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;
u32 val, ch_base;
bld_base = sun8i_blender_base(mixer); ch_base = sun8i_channel_base(mixer, channel); DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",@@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel, @@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state) { struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false,0);
}
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, @@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, { struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_ui_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, layer->channel, layer->overlay, plane); sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = { diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -17,8 +17,7 @@ #include "sun8i_vi_scaler.h"
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{ u32 val, bld_base, ch_base;
@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel, @@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state) { struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false,0);
}
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, @@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, { struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_vi_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, layer->channel, layer->overlay, plane); sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko napisal(a):
Hello Jernej,
Thank you for review.
On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je
roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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;int channel_by_zpos[4] = {-1, -1, -1, -1};u32 route = 0, pipe_ctl = 0;DRM_DEBUG_DRIVER("Update blender routing\n");Use drm_dbg().
for (i = 0; i < 4; i++) {int zpos = mixer->channel_zpos[i];channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
I'll use plane_cnt as it done in mixer_bind
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
Will do.
if (zpos >= 0 && zpos < 4)channel_by_zpos[zpos] = i;}j = 0;for (i = 0; i < 4; 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++;}}for (i = 0; i < 4 && j < 4; i++) {int zpos = mixer->channel_zpos[i];if (zpos < 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");Use drm_dbg().
According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3beb 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 , DRM_DEBUG_DRIVER uses drm_dbg. Also, using drm_dbg with category macro would require larger indent, making harder to fit in 80 chars limit.
From what I can see, category is already defined by macro name. Check here: https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n465
So it should be actually shorter.
Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
Yes, at least I understand following commit message in such manner: https://cgit.freedesktop.org/drm/drm-misc/commit/? id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
Best regards, Jernej
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF, SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);}
@@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, mixer->engine.ops = &sun8i_engine_ops;
mixer->engine.node = dev->of_node;
mixer->channel_zpos[0] = -1;mixer->channel_zpos[1] = -1;mixer->channel_zpos[2] = -1;mixer->channel_zpos[3] = -1;mixer->channel_zpos[4] = -1;for loop would be better, especially using proposed macro.
I'll put it into already existent for-loop below.
Best regards, Jernej
/* * While this function can fail, we shouldn't do anything * if this happens. Some early DE2 DT entries don't providediff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -178,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk; struct clk *mod_clk;
/* -1 means that layer is disabled */int channel_zpos[5];};
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 893076716070..23c2f4b68c89 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,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;
u32 val, ch_base;
bld_base = sun8i_blender_base(mixer); ch_base = sun8i_channel_base(mixer, channel); DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",@@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
@@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_ui_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -17,8 +17,7 @@
#include "sun8i_vi_scaler.h"
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
@@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_vi_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec jernej.skrabec@siol.net wrote:
Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko napisal(a):
Hello Jernej,
Thank you for review.
On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je
roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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;int channel_by_zpos[4] = {-1, -1, -1, -1};u32 route = 0, pipe_ctl = 0;DRM_DEBUG_DRIVER("Update blender routing\n");Use drm_dbg().
for (i = 0; i < 4; i++) {int zpos = mixer->channel_zpos[i];channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
I'll use plane_cnt as it done in mixer_bind
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
Will do.
if (zpos >= 0 && zpos < 4)channel_by_zpos[zpos] = i;}j = 0;for (i = 0; i < 4; 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++;}}for (i = 0; i < 4 && j < 4; i++) {int zpos = mixer->channel_zpos[i];if (zpos < 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");Use drm_dbg().
According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3beb 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 , DRM_DEBUG_DRIVER uses drm_dbg. Also, using drm_dbg with category macro would require larger indent, making harder to fit in 80 chars limit.
From what I can see, category is already defined by macro name. Check here: https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n465
So it should be actually shorter.
Ah, it something very recent. drm_dbg also require drm_device struct Do you know the best way to extract it from `struct engine`?
Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
Yes, at least I understand following commit message in such manner: https://cgit.freedesktop.org/drm/drm-misc/commit/? id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
Best regards, Jernej
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF, SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);}
@@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, mixer->engine.ops = &sun8i_engine_ops;
mixer->engine.node = dev->of_node;
mixer->channel_zpos[0] = -1;mixer->channel_zpos[1] = -1;mixer->channel_zpos[2] = -1;mixer->channel_zpos[3] = -1;mixer->channel_zpos[4] = -1;for loop would be better, especially using proposed macro.
I'll put it into already existent for-loop below.
Best regards, Jernej
/* * While this function can fail, we shouldn't do anything * if this happens. Some early DE2 DT entries don't providediff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -178,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk; struct clk *mod_clk;
/* -1 means that layer is disabled */int channel_zpos[5];};
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 893076716070..23c2f4b68c89 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,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;
u32 val, ch_base;
bld_base = sun8i_blender_base(mixer); ch_base = sun8i_channel_base(mixer, channel); DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",@@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
@@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_ui_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -17,8 +17,7 @@
#include "sun8i_vi_scaler.h"
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
@@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_vi_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko napisal(a):
On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
napisal(a):
Hello Jernej,
Thank you for review.
On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je
roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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;int channel_by_zpos[4] = {-1, -1, -1, -1};u32 route = 0, pipe_ctl = 0;DRM_DEBUG_DRIVER("Update blender routing\n");Use drm_dbg().
for (i = 0; i < 4; i++) {int zpos = mixer->channel_zpos[i];channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
I'll use plane_cnt as it done in mixer_bind
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
Will do.
if (zpos >= 0 && zpos < 4)channel_by_zpos[zpos] = i;}j = 0;for (i = 0; i < 4; 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++;}}for (i = 0; i < 4 && j < 4; i++) {int zpos = mixer->channel_zpos[i];if (zpos < 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");Use drm_dbg().
According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3 beb 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 , DRM_DEBUG_DRIVER uses drm_dbg. Also, using drm_dbg with category macro would require larger indent, making harder to fit in 80 chars limit.
From what I can see, category is already defined by macro name. Check here: https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n46 5
So it should be actually shorter.
Ah, it something very recent. drm_dbg also require drm_device struct Do you know the best way to extract it from `struct engine`?
I don't think there is a way. I guess we can solve this later. Maxime, any thoughts?
Best regards, Jernej
Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
Yes, at least I understand following commit message in such manner: https://cgit.freedesktop.org/drm/drm-misc/commit/? id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
Best regards, Jernej
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF, SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);}
@@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, mixer->engine.ops = &sun8i_engine_ops;
mixer->engine.node = dev->of_node;
mixer->channel_zpos[0] = -1;mixer->channel_zpos[1] = -1;mixer->channel_zpos[2] = -1;mixer->channel_zpos[3] = -1;mixer->channel_zpos[4] = -1;for loop would be better, especially using proposed macro.
I'll put it into already existent for-loop below.
Best regards, Jernej
/* * While this function can fail, we shouldn't do anything * if this happens. Some early DE2 DT entries don't providediff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -178,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk; struct clk *mod_clk;
/* -1 means that layer is disabled */int channel_zpos[5];};
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 893076716070..23c2f4b68c89 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,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;
u32 val, ch_base;
bld_base = sun8i_blender_base(mixer); ch_base = sun8i_channel_base(mixer, channel); DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",@@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
@@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_ui_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -17,8 +17,7 @@
#include "sun8i_vi_scaler.h"
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
@@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_vi_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
My proposal is to go with DRM_DEBUG_DRIVER for now. In case stable branches would be interested in these fixes, it will be easier to backport.
Also I am using v5.4 for testing since Google AOSP patches does not work correctly with mainline and missing for kernel-next. Maintaining 2 variants will be much harder for me.
On Sun, Dec 29, 2019 at 3:02 PM Jernej Škrabec jernej.skrabec@siol.net wrote:
Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko napisal(a):
On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
napisal(a):
Hello Jernej,
Thank you for review.
On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je
roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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;int channel_by_zpos[4] = {-1, -1, -1, -1};u32 route = 0, pipe_ctl = 0;DRM_DEBUG_DRIVER("Update blender routing\n");Use drm_dbg().
for (i = 0; i < 4; i++) {int zpos = mixer->channel_zpos[i];channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
I'll use plane_cnt as it done in mixer_bind
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
Will do.
if (zpos >= 0 && zpos < 4)channel_by_zpos[zpos] = i;}j = 0;for (i = 0; i < 4; 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++;}}for (i = 0; i < 4 && j < 4; i++) {int zpos = mixer->channel_zpos[i];if (zpos < 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");Use drm_dbg().
According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3 beb 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 , DRM_DEBUG_DRIVER uses drm_dbg. Also, using drm_dbg with category macro would require larger indent, making harder to fit in 80 chars limit.
From what I can see, category is already defined by macro name. Check here: https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n46 5
So it should be actually shorter.
Ah, it something very recent. drm_dbg also require drm_device struct Do you know the best way to extract it from `struct engine`?
I don't think there is a way. I guess we can solve this later. Maxime, any thoughts?
Best regards, Jernej
Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
Yes, at least I understand following commit message in such manner: https://cgit.freedesktop.org/drm/drm-misc/commit/? id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
Best regards, Jernej
regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF, SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);}
@@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, mixer->engine.ops = &sun8i_engine_ops;
mixer->engine.node = dev->of_node;
mixer->channel_zpos[0] = -1;mixer->channel_zpos[1] = -1;mixer->channel_zpos[2] = -1;mixer->channel_zpos[3] = -1;mixer->channel_zpos[4] = -1;for loop would be better, especially using proposed macro.
I'll put it into already existent for-loop below.
Best regards, Jernej
/* * While this function can fail, we shouldn't do anything * if this happens. Some early DE2 DT entries don't providediff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 915479cc3077..9c2ff87923d8 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -178,6 +178,9 @@ struct sun8i_mixer {
struct clk *bus_clk; struct clk *mod_clk;
/* -1 means that layer is disabled */int channel_zpos[5];};
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 893076716070..23c2f4b68c89 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,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;
u32 val, ch_base;
bld_base = sun8i_blender_base(mixer); ch_base = sun8i_channel_base(mixer, channel); DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n",@@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -235,11 +208,9 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
@@ -247,12 +218,11 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_ui_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -263,7 +233,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 42d445d23773..97cbc98bf781 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -17,8 +17,7 @@
#include "sun8i_vi_scaler.h"
static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
int overlay, bool enable,unsigned int zpos,
unsigned int old_zpos)
int overlay, bool enable,unsigned int zpos)
{
u32 val, bld_base, ch_base;@@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
if (!enable || zpos != old_zpos) {regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
0);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
0);}if (enable) {val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
val, val);val = channel <<SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
regmap_update_bits(mixer->engine.regs,SUN8I_MIXER_BLEND_ROUTE(bld_base),
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
val);}
mixer->channel_zpos[channel] = enable ? zpos : -1;}
static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
channel, @@ -350,11 +324,9 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state)
{
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer;sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false, 0,
old_zpos);
sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,false,
0);
}
static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
@@ -362,12 +334,11 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, {
struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane); unsigned int zpos = plane->state->normalized_zpos;
unsigned int old_zpos = old_state->normalized_zpos; struct sun8i_mixer *mixer = layer->mixer; if (!plane->state->visible) { sun8i_vi_layer_enable(mixer, layer->channel,layer->overlay, false, 0,old_zpos);
layer->overlay, false, 0); return; }@@ -378,7 +349,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, layer->channel,
layer->overlay, plane); sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
true, zpos, old_zpos);
true, zpos);}
static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
Dne nedelja, 29. december 2019 ob 14:13:06 CET je Roman Stratiienko napisal(a):
My proposal is to go with DRM_DEBUG_DRIVER for now. In case stable branches would be interested in these fixes, it will be easier to backport.
Fair enough, but you need to add at least Fixes tag.
Best regards, Jernej
Also I am using v5.4 for testing since Google AOSP patches does not work correctly with mainline and missing for kernel-next. Maintaining 2 variants will be much harder for me.
On Sun, Dec 29, 2019 at 3:02 PM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko
napisal(a):
On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
napisal(a):
Hello Jernej,
Thank you for review.
On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je
roman.stratiienko@globallogic.com napisal(a): > From: Roman Stratiienko roman.stratiienko@globallogic.com > > 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. > > Signed-off-by: Roman Stratiienko > roman.stratiienko@globallogic.com > --- > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 > +++++++++++++++++++++++++- > drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 > ++++------------------- > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 > +++------------------ > 4 files changed, 60 insertions(+), 71 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index > bb9a665fd053..da84fccf7784 > 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct > sunxi_engine > *engine, > > 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; > + int channel_by_zpos[4] = {-1, -1, -1, -1}; > + u32 route = 0, pipe_ctl = 0; > + > + DRM_DEBUG_DRIVER("Update blender routing\n");
Use drm_dbg().
> + for (i = 0; i < 4; i++) { > + int zpos = mixer->channel_zpos[i];
channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
I'll use plane_cnt as it done in mixer_bind
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
Will do.
> + > + if (zpos >= 0 && zpos < 4) > + channel_by_zpos[zpos] = i; > + } > + > + j = 0; > + for (i = 0; i < 4; 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++; > + } > + } > + > + for (i = 0; i < 4 && j < 4; i++) { > + int zpos = mixer->channel_zpos[i]; > > + if (zpos < 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");
Use drm_dbg().
According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c80584765755 93b3 beb 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 , DRM_DEBUG_DRIVER uses drm_dbg. Also, using drm_dbg with category macro would require larger indent, making harder to fit in 80 chars limit.
From what I can see, category is already defined by macro name. Check here: https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h #n46 5
So it should be actually shorter.
Ah, it something very recent. drm_dbg also require drm_device struct Do you know the best way to extract it from `struct engine`?
I don't think there is a way. I guess we can solve this later. Maxime, any thoughts?
Best regards, Jernej
Are there any plans to deprecate DRM_DEBUG_DRIVER macro?
Yes, at least I understand following commit message in such manner: https://cgit.freedesktop.org/drm/drm-misc/commit/? id=93ccfa9a4eca482216c5caf88be77e5ffa0d744a
Best regards, Jernej
> regmap_write(engine->regs, SUN8I_MIXER_GLOBAL_DBUFF, > > SUN8I_MIXER_GLOBAL_DBUFF_ENABLE); > > } > > @@ -422,6 +461,12 @@ static int sun8i_mixer_bind(struct device > *dev, > struct > device *master, mixer->engine.ops = &sun8i_engine_ops; > > mixer->engine.node = dev->of_node; > > + mixer->channel_zpos[0] = -1; > + mixer->channel_zpos[1] = -1; > + mixer->channel_zpos[2] = -1; > + mixer->channel_zpos[3] = -1; > + mixer->channel_zpos[4] = -1; > +
for loop would be better, especially using proposed macro.
I'll put it into already existent for-loop below.
Best regards, Jernej
> /* > > * While this function can fail, we shouldn't do anything > * if this happens. Some early DE2 DT entries don't > provide > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index > 915479cc3077..9c2ff87923d8 > 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > @@ -178,6 +178,9 @@ struct sun8i_mixer { > > struct clk *bus_clk; > struct clk *mod_clk; > > + > + /* -1 means that layer is disabled */ > + int channel_zpos[5]; > > }; > > 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 > 893076716070..23c2f4b68c89 > 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, > > - int overlay, bool enable,
unsigned int zpos,
> - unsigned int old_zpos) > + int overlay, bool enable,
unsigned int zpos)
> { > > - u32 val, bld_base, ch_base; > + u32 val, ch_base; > > - bld_base = sun8i_blender_base(mixer); > > ch_base = sun8i_channel_base(mixer, channel); > > DRM_DEBUG_DRIVER("%sabling channel %d overlay %d\n", > > @@ -44,32 +42,7 @@ static void sun8i_ui_layer_enable(struct > sun8i_mixer > *mixer, int channel, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, > overlay), > > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, > val); > > - if (!enable || zpos != old_zpos) { > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> -
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> - 0); > - > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_ROUTE(bld_base),
> -
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> - 0); > - } > - > - if (enable) { > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos); > - > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> - val, val); > - > - val = channel <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> - > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_ROUTE(bld_base),
> -
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> - val); > - } > + mixer->channel_zpos[channel] = enable ? zpos : -1; > > } > > static int sun8i_ui_layer_update_coord(struct sun8i_mixer > *mixer, > int > > channel, @@ -235,11 +208,9 @@ static void > sun8i_ui_layer_atomic_disable(struct drm_plane *plane, struct > drm_plane_state *old_state) > > { > > struct sun8i_ui_layer *layer = > plane_to_sun8i_ui_layer(plane); > > - unsigned int old_zpos = old_state->normalized_zpos; > > struct sun8i_mixer *mixer = layer->mixer; > > - sun8i_ui_layer_enable(mixer, layer->channel, > layer->overlay,
false, 0,
> - old_zpos); > + sun8i_ui_layer_enable(mixer, layer->channel, > layer->overlay, > false,
0);
> } > > static void sun8i_ui_layer_atomic_update(struct drm_plane > *plane, > > @@ -247,12 +218,11 @@ static void > sun8i_ui_layer_atomic_update(struct > drm_plane *plane, { > > struct sun8i_ui_layer *layer = > plane_to_sun8i_ui_layer(plane); > unsigned int zpos = plane->state->normalized_zpos; > > - unsigned int old_zpos = old_state->normalized_zpos; > > struct sun8i_mixer *mixer = layer->mixer; > > if (!plane->state->visible) { > > sun8i_ui_layer_enable(mixer, layer->channel, > > - layer->overlay, false, 0,
old_zpos);
> + layer->overlay, false, 0); > > return; > > } > > @@ -263,7 +233,7 @@ static void > sun8i_ui_layer_atomic_update(struct > drm_plane *plane, sun8i_ui_layer_update_buffer(mixer, > layer->channel, > > layer->overlay, plane); > > sun8i_ui_layer_enable(mixer, layer->channel, > layer->overlay, > > - true, zpos, old_zpos); > + true, zpos); > > } > > static struct drm_plane_helper_funcs > sun8i_ui_layer_helper_funcs = > { > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index > 42d445d23773..97cbc98bf781 > 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c > @@ -17,8 +17,7 @@ > > #include "sun8i_vi_scaler.h" > > static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, > int > channel, > > - int overlay, bool enable,
unsigned int zpos,
> - unsigned int old_zpos) > + int overlay, bool enable,
unsigned int zpos)
> { > > u32 val, bld_base, ch_base; > > @@ -37,32 +36,7 @@ static void sun8i_vi_layer_enable(struct > sun8i_mixer > *mixer, int channel, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, > overlay), > > SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, > val); > > - if (!enable || zpos != old_zpos) { > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> -
SUN8I_MIXER_BLEND_PIPE_CTL_EN(old_zpos),
> - 0); > - > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_ROUTE(bld_base),
> -
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(old_zpos),
> - 0); > - } > - > - if (enable) { > - val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos); > - > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_PIPE_CTL(bld_base),
> - val, val); > - > - val = channel <<
SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> - > - regmap_update_bits(mixer->engine.regs, > -
SUN8I_MIXER_BLEND_ROUTE(bld_base),
> -
SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> - val); > - } > + mixer->channel_zpos[channel] = enable ? zpos : -1; > > } > > static int sun8i_vi_layer_update_coord(struct sun8i_mixer > *mixer, > int > > channel, @@ -350,11 +324,9 @@ static void > sun8i_vi_layer_atomic_disable(struct drm_plane *plane, struct > drm_plane_state *old_state) > > { > > struct sun8i_vi_layer *layer = > plane_to_sun8i_vi_layer(plane); > > - unsigned int old_zpos = old_state->normalized_zpos; > > struct sun8i_mixer *mixer = layer->mixer; > > - sun8i_vi_layer_enable(mixer, layer->channel, > layer->overlay,
false, 0,
> - old_zpos); > + sun8i_vi_layer_enable(mixer, layer->channel, > layer->overlay, > false,
0);
> } > > static void sun8i_vi_layer_atomic_update(struct drm_plane > *plane, > > @@ -362,12 +334,11 @@ static void > sun8i_vi_layer_atomic_update(struct > drm_plane *plane, { > > struct sun8i_vi_layer *layer = > plane_to_sun8i_vi_layer(plane); > unsigned int zpos = plane->state->normalized_zpos; > > - unsigned int old_zpos = old_state->normalized_zpos; > > struct sun8i_mixer *mixer = layer->mixer; > > if (!plane->state->visible) { > > sun8i_vi_layer_enable(mixer, layer->channel, > > - layer->overlay, false, 0,
old_zpos);
> + layer->overlay, false, 0); > > return; > > } > > @@ -378,7 +349,7 @@ static void > sun8i_vi_layer_atomic_update(struct > drm_plane *plane, sun8i_vi_layer_update_buffer(mixer, > layer->channel, > > layer->overlay, plane); > > sun8i_vi_layer_enable(mixer, layer->channel, > layer->overlay, > > - true, zpos, old_zpos); > + true, zpos); > > } > > static struct drm_plane_helper_funcs > sun8i_vi_layer_helper_funcs = > {
On Sun, Dec 29, 2019 at 02:02:33PM +0100, Jernej Škrabec wrote:
Dne nedelja, 29. december 2019 ob 13:47:38 CET je Roman Stratiienko napisal(a):
On Sun, Dec 29, 2019 at 2:18 PM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Dne nedelja, 29. december 2019 ob 13:08:19 CET je Roman Stratiienko
napisal(a):
Hello Jernej,
Thank you for review.
On Sun, Dec 29, 2019 at 11:40 AM Jernej Škrabec jernej.skrabec@siol.net
wrote:
Hi!
Dne sobota, 28. december 2019 ob 21:28:17 CET je
roman.stratiienko@globallogic.com napisal(a):
From: Roman Stratiienko roman.stratiienko@globallogic.com
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.
Signed-off-by: Roman Stratiienko roman.stratiienko@globallogic.com
drivers/gpu/drm/sun4i/sun8i_mixer.c | 47 +++++++++++++++++++++++++- drivers/gpu/drm/sun4i/sun8i_mixer.h | 3 ++ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 42 ++++------------------- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 39 +++------------------ 4 files changed, 60 insertions(+), 71 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index bb9a665fd053..da84fccf7784 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -307,8 +307,47 @@ static void sun8i_atomic_begin(struct sunxi_engine *engine,
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;int channel_by_zpos[4] = {-1, -1, -1, -1};u32 route = 0, pipe_ctl = 0;DRM_DEBUG_DRIVER("Update blender routing\n");Use drm_dbg().
for (i = 0; i < 4; i++) {int zpos = mixer->channel_zpos[i];channel_zpos can hold 5 elements which is also theoretical maximum for current HW design. Why do you check only 4 elements?
I'll use plane_cnt as it done in mixer_bind
It would be great to introduce a macro like SUN8I_MIXER_MAX_LAYERS so everyone would understand where this number comes from.
Will do.
if (zpos >= 0 && zpos < 4)channel_by_zpos[zpos] = i;}j = 0;for (i = 0; i < 4; 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++;}}for (i = 0; i < 4 && j < 4; i++) {int zpos = mixer->channel_zpos[i];if (zpos < 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");Use drm_dbg().
According to https://github.com/torvalds/linux/commit/99a954874e7b9f0c8058476575593b3 beb 5731a5#diff-b0cd2d683c6afbab7bd54173cfd3d3ecR289 , DRM_DEBUG_DRIVER uses drm_dbg. Also, using drm_dbg with category macro would require larger indent, making harder to fit in 80 chars limit.
From what I can see, category is already defined by macro name. Check here: https://cgit.freedesktop.org/drm/drm-misc/tree/include/drm/drm_print.h#n46 5
So it should be actually shorter.
Ah, it something very recent. drm_dbg also require drm_device struct Do you know the best way to extract it from `struct engine`?
I don't think there is a way. I guess we can solve this later. Maxime, any thoughts?
There's no way at the moment, but it would make sense to add a pointer to it.
Maximey
dri-devel@lists.freedesktop.org
-
Jernej Škrabec -
Maxime Ripard -
Roman Stratiienko -
roman.stratiienko＠globallogic.com