* [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
@ 2016-09-19 14:16 ` Tobias Jakobi
2016-09-20 10:00 ` Andrzej Hajda
0 siblings, 1 reply; 7+ messages in thread
From: Tobias Jakobi @ 2016-09-19 14:16 UTC (permalink / raw)
To: linux-samsung-soc; +Cc: Tobias Jakobi, dri-devel, m.szyprowski
Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once
in mixer_cfg_layer().
Trigger this via atomic flush.
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------
drivers/gpu/drm/exynos/regs-mixer.h | 2 +
2 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 1e78d57..d4efd9c 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -99,6 +99,7 @@ struct mixer_context {
struct drm_device *drm_dev;
struct exynos_drm_crtc *crtc;
struct exynos_drm_plane planes[MIXER_WIN_NR];
+ unsigned long state_cache;
int pipe;
unsigned long flags;
@@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
}
-static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
- unsigned int priority, bool enable)
+static void mixer_cfg_layer(struct mixer_context *ctx)
{
struct mixer_resources *res = &ctx->mixer_res;
- u32 val = enable ? ~0 : 0;
-
- switch (win) {
- case 0:
- mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
- mixer_reg_writemask(res, MXR_LAYER_CFG,
- MXR_LAYER_CFG_GRP0_VAL(priority),
- MXR_LAYER_CFG_GRP0_MASK);
- break;
- case 1:
- mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
- mixer_reg_writemask(res, MXR_LAYER_CFG,
- MXR_LAYER_CFG_GRP1_VAL(priority),
- MXR_LAYER_CFG_GRP1_MASK);
+ unsigned int win;
- break;
- case VP_DEFAULT_WIN:
- if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
- vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
- mixer_reg_writemask(res, MXR_CFG, val,
- MXR_CFG_VP_ENABLE);
- mixer_reg_writemask(res, MXR_LAYER_CFG,
- MXR_LAYER_CFG_VP_VAL(priority),
- MXR_LAYER_CFG_VP_MASK);
+ struct exynos_drm_plane_state *state;
+ struct drm_framebuffer *fb;
+ unsigned int priority;
+ u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
+ bool enable;
+
+ for (win = 0; win < MIXER_WIN_NR; ++win) {
+ state = to_exynos_plane_state(ctx->planes[win].base.state);
+ fb = state->fb;
+
+ priority = state->base.normalized_zpos + 1;
+ enable = test_bit(win, &ctx->state_cache);
+
+ if (!enable)
+ continue;
+
+ switch (win) {
+ case 0:
+ mxr_cfg |= MXR_CFG_GRP0_ENABLE;
+ mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);
+ break;
+
+ case 1:
+ mxr_cfg |= MXR_CFG_GRP1_ENABLE;
+ mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);
+ break;
+
+ case VP_DEFAULT_WIN:
+ vp_enable = VP_ENABLE_ON;
+ mxr_cfg |= MXR_CFG_VP_ENABLE;
+ mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);
+ break;
+ }
+
+ if (!fb)
+ continue;
+
+ /*
+ * TODO: Don't enable alpha blending for the bottom window.
+ */
+ switch (win) {
+ case 0:
+ case 1:
+ mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
+ break;
+
+ case VP_DEFAULT_WIN:
+ mixer_cfg_vp_blend(ctx);
+ break;
}
- break;
}
+
+ if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
+ vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);
+
+ mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
+ mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
}
static void mixer_run(struct mixer_context *ctx)
@@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
struct mixer_resources *res = &ctx->mixer_res;
struct drm_framebuffer *fb = state->fb;
- unsigned int priority = state->base.normalized_zpos + 1;
unsigned long flags;
dma_addr_t luma_addr[2], chroma_addr[2];
bool tiled_mode = false;
@@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay);
mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
- mixer_cfg_layer(ctx, plane->index, priority, true);
- mixer_cfg_vp_blend(ctx);
mixer_run(ctx);
spin_unlock_irqrestore(&res->reg_slock, flags);
@@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
struct mixer_resources *res = &ctx->mixer_res;
struct drm_framebuffer *fb = state->fb;
- unsigned int priority = state->base.normalized_zpos + 1;
unsigned long flags;
unsigned int win = plane->index;
unsigned int x_ratio = 0, y_ratio = 0;
@@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
mixer_cfg_scan(ctx, mode->vdisplay);
mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
- mixer_cfg_layer(ctx, win, priority, true);
- mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
/* layer update mandatory for mixer 16.0.33.0 */
if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
@@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
vp_video_buffer(mixer_ctx, plane);
else
mixer_graph_buffer(mixer_ctx, plane);
+
+ __set_bit(plane->index, &mixer_ctx->state_cache);
}
static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
struct exynos_drm_plane *plane)
{
struct mixer_context *mixer_ctx = crtc->ctx;
- struct mixer_resources *res = &mixer_ctx->mixer_res;
- unsigned long flags;
DRM_DEBUG_KMS("win: %d\n", plane->index);
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
return;
- spin_lock_irqsave(&res->reg_slock, flags);
- mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
- spin_unlock_irqrestore(&res->reg_slock, flags);
+ __clear_bit(plane->index, &mixer_ctx->state_cache);
}
static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
{
struct mixer_context *mixer_ctx = crtc->ctx;
+ struct mixer_resources *res = &mixer_ctx->mixer_res;
+ unsigned long flags;
if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
return;
+ spin_lock_irqsave(&res->reg_slock, flags);
+ mixer_cfg_layer(mixer_ctx);
+ spin_unlock_irqrestore(&res->reg_slock, flags);
+
mixer_vsync_set_update(mixer_ctx, true);
}
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index 7f22df5..728a18e 100644
--- a/drivers/gpu/drm/exynos/regs-mixer.h
+++ b/drivers/gpu/drm/exynos/regs-mixer.h
@@ -100,6 +100,7 @@
#define MXR_CFG_GRP1_ENABLE (1 << 5)
#define MXR_CFG_GRP0_ENABLE (1 << 4)
#define MXR_CFG_VP_ENABLE (1 << 3)
+#define MXR_CFG_ENABLE_MASK (0x7 << 3)
#define MXR_CFG_SCAN_INTERLACE (0 << 2)
#define MXR_CFG_SCAN_PROGRESSIVE (1 << 2)
#define MXR_CFG_SCAN_NTSC (0 << 1)
@@ -151,6 +152,7 @@
#define MXR_LAYER_CFG_GRP0_MASK MXR_LAYER_CFG_GRP0_VAL(~0)
#define MXR_LAYER_CFG_VP_VAL(x) MXR_MASK_VAL(x, 3, 0)
#define MXR_LAYER_CFG_VP_MASK MXR_LAYER_CFG_VP_VAL(~0)
+#define MXR_LAYER_CFG_MASK 0xFFF
#endif /* SAMSUNG_REGS_MIXER_H */
--
2.7.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
2016-09-19 14:16 ` [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush() Tobias Jakobi
@ 2016-09-20 10:00 ` Andrzej Hajda
2016-09-20 11:23 ` Tobias Jakobi
0 siblings, 1 reply; 7+ messages in thread
From: Andrzej Hajda @ 2016-09-20 10:00 UTC (permalink / raw)
To: Tobias Jakobi, linux-samsung-soc; +Cc: dri-devel, m.szyprowski
On 19.09.2016 16:16, Tobias Jakobi wrote:
> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once
> in mixer_cfg_layer().
> Trigger this via atomic flush.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
> drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------
> drivers/gpu/drm/exynos/regs-mixer.h | 2 +
> 2 files changed, 69 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 1e78d57..d4efd9c 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -99,6 +99,7 @@ struct mixer_context {
> struct drm_device *drm_dev;
> struct exynos_drm_crtc *crtc;
> struct exynos_drm_plane planes[MIXER_WIN_NR];
> + unsigned long state_cache;
The name of the variable is cryptic.
> int pipe;
> unsigned long flags;
>
> @@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
> }
>
> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
> - unsigned int priority, bool enable)
> +static void mixer_cfg_layer(struct mixer_context *ctx)
> {
> struct mixer_resources *res = &ctx->mixer_res;
> - u32 val = enable ? ~0 : 0;
> -
> - switch (win) {
> - case 0:
> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
> - mixer_reg_writemask(res, MXR_LAYER_CFG,
> - MXR_LAYER_CFG_GRP0_VAL(priority),
> - MXR_LAYER_CFG_GRP0_MASK);
> - break;
> - case 1:
> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
> - mixer_reg_writemask(res, MXR_LAYER_CFG,
> - MXR_LAYER_CFG_GRP1_VAL(priority),
> - MXR_LAYER_CFG_GRP1_MASK);
> + unsigned int win;
>
> - break;
> - case VP_DEFAULT_WIN:
> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
> - mixer_reg_writemask(res, MXR_CFG, val,
> - MXR_CFG_VP_ENABLE);
> - mixer_reg_writemask(res, MXR_LAYER_CFG,
> - MXR_LAYER_CFG_VP_VAL(priority),
> - MXR_LAYER_CFG_VP_MASK);
> + struct exynos_drm_plane_state *state;
> + struct drm_framebuffer *fb;
> + unsigned int priority;
> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
> + bool enable;
> +
> + for (win = 0; win < MIXER_WIN_NR; ++win) {
> + state = to_exynos_plane_state(ctx->planes[win].base.state);
> + fb = state->fb;
> +
> + priority = state->base.normalized_zpos + 1;
> + enable = test_bit(win, &ctx->state_cache);
> +
> + if (!enable)
> + continue;
> +
> + switch (win) {
> + case 0:
> + mxr_cfg |= MXR_CFG_GRP0_ENABLE;
> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);
> + break;
> +
> + case 1:
> + mxr_cfg |= MXR_CFG_GRP1_ENABLE;
> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);
> + break;
> +
> + case VP_DEFAULT_WIN:
> + vp_enable = VP_ENABLE_ON;
> + mxr_cfg |= MXR_CFG_VP_ENABLE;
> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);
> + break;
> + }
> +
> + if (!fb)
> + continue;
> +
> + /*
> + * TODO: Don't enable alpha blending for the bottom window.
> + */
> + switch (win) {
> + case 0:
> + case 1:
> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
> + break;
> +
> + case VP_DEFAULT_WIN:
> + mixer_cfg_vp_blend(ctx);
> + break;
> }
> - break;
> }
> +
> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);
> +
> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
What about enabled planes which are not updated?
Corresponding bit in ctx->state_cache will be 0.
It looks like this routine will disable them in hardware, am I right?
Regards
Andrzej
> }
>
> static void mixer_run(struct mixer_context *ctx)
> @@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
> struct mixer_resources *res = &ctx->mixer_res;
> struct drm_framebuffer *fb = state->fb;
> - unsigned int priority = state->base.normalized_zpos + 1;
> unsigned long flags;
> dma_addr_t luma_addr[2], chroma_addr[2];
> bool tiled_mode = false;
> @@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>
> mixer_cfg_scan(ctx, mode->vdisplay);
> mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
> - mixer_cfg_layer(ctx, plane->index, priority, true);
> - mixer_cfg_vp_blend(ctx);
> mixer_run(ctx);
>
> spin_unlock_irqrestore(&res->reg_slock, flags);
> @@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
> struct mixer_resources *res = &ctx->mixer_res;
> struct drm_framebuffer *fb = state->fb;
> - unsigned int priority = state->base.normalized_zpos + 1;
> unsigned long flags;
> unsigned int win = plane->index;
> unsigned int x_ratio = 0, y_ratio = 0;
> @@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>
> mixer_cfg_scan(ctx, mode->vdisplay);
> mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
> - mixer_cfg_layer(ctx, win, priority, true);
> - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>
> /* layer update mandatory for mixer 16.0.33.0 */
> if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
> @@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
> vp_video_buffer(mixer_ctx, plane);
> else
> mixer_graph_buffer(mixer_ctx, plane);
> +
> + __set_bit(plane->index, &mixer_ctx->state_cache);
> }
>
> static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
> struct exynos_drm_plane *plane)
> {
> struct mixer_context *mixer_ctx = crtc->ctx;
> - struct mixer_resources *res = &mixer_ctx->mixer_res;
> - unsigned long flags;
>
> DRM_DEBUG_KMS("win: %d\n", plane->index);
>
> if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
> return;
>
> - spin_lock_irqsave(&res->reg_slock, flags);
> - mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
> - spin_unlock_irqrestore(&res->reg_slock, flags);
> + __clear_bit(plane->index, &mixer_ctx->state_cache);
> }
>
> static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
> {
> struct mixer_context *mixer_ctx = crtc->ctx;
> + struct mixer_resources *res = &mixer_ctx->mixer_res;
> + unsigned long flags;
>
> if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
> return;
>
> + spin_lock_irqsave(&res->reg_slock, flags);
> + mixer_cfg_layer(mixer_ctx);
> + spin_unlock_irqrestore(&res->reg_slock, flags);
> +
> mixer_vsync_set_update(mixer_ctx, true);
> }
>
> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
> index 7f22df5..728a18e 100644
> --- a/drivers/gpu/drm/exynos/regs-mixer.h
> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
> @@ -100,6 +100,7 @@
> #define MXR_CFG_GRP1_ENABLE (1 << 5)
> #define MXR_CFG_GRP0_ENABLE (1 << 4)
> #define MXR_CFG_VP_ENABLE (1 << 3)
> +#define MXR_CFG_ENABLE_MASK (0x7 << 3)
> #define MXR_CFG_SCAN_INTERLACE (0 << 2)
> #define MXR_CFG_SCAN_PROGRESSIVE (1 << 2)
> #define MXR_CFG_SCAN_NTSC (0 << 1)
> @@ -151,6 +152,7 @@
> #define MXR_LAYER_CFG_GRP0_MASK MXR_LAYER_CFG_GRP0_VAL(~0)
> #define MXR_LAYER_CFG_VP_VAL(x) MXR_MASK_VAL(x, 3, 0)
> #define MXR_LAYER_CFG_VP_MASK MXR_LAYER_CFG_VP_VAL(~0)
> +#define MXR_LAYER_CFG_MASK 0xFFF
>
> #endif /* SAMSUNG_REGS_MIXER_H */
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
2016-09-20 10:00 ` Andrzej Hajda
@ 2016-09-20 11:23 ` Tobias Jakobi
2016-09-20 12:34 ` Andrzej Hajda
0 siblings, 1 reply; 7+ messages in thread
From: Tobias Jakobi @ 2016-09-20 11:23 UTC (permalink / raw)
To: Andrzej Hajda, linux-samsung-soc; +Cc: dri-devel, m.szyprowski
Hello Andrzej,
first of all, I've noticed an error myself. mixer_disable() calls
mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
Now to your points...
Andrzej Hajda wrote:
> On 19.09.2016 16:16, Tobias Jakobi wrote:
>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once
>> in mixer_cfg_layer().
>> Trigger this via atomic flush.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>> drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------
>> drivers/gpu/drm/exynos/regs-mixer.h | 2 +
>> 2 files changed, 69 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 1e78d57..d4efd9c 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -99,6 +99,7 @@ struct mixer_context {
>> struct drm_device *drm_dev;
>> struct exynos_drm_crtc *crtc;
>> struct exynos_drm_plane planes[MIXER_WIN_NR];
>> + unsigned long state_cache;
>
> The name of the variable is cryptic.
Yes, I'll try to come up with something better. It would probably be
easier if struct mixer_context had a documentation for its fields.
Anyway, any suggestions?
>
>> int pipe;
>> unsigned long flags;
>>
>> @@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
>> }
>>
>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>> - unsigned int priority, bool enable)
>> +static void mixer_cfg_layer(struct mixer_context *ctx)
>> {
>> struct mixer_resources *res = &ctx->mixer_res;
>> - u32 val = enable ? ~0 : 0;
>> -
>> - switch (win) {
>> - case 0:
>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>> - MXR_LAYER_CFG_GRP0_VAL(priority),
>> - MXR_LAYER_CFG_GRP0_MASK);
>> - break;
>> - case 1:
>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>> - MXR_LAYER_CFG_GRP1_VAL(priority),
>> - MXR_LAYER_CFG_GRP1_MASK);
>> + unsigned int win;
>>
>> - break;
>> - case VP_DEFAULT_WIN:
>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
>> - mixer_reg_writemask(res, MXR_CFG, val,
>> - MXR_CFG_VP_ENABLE);
>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>> - MXR_LAYER_CFG_VP_VAL(priority),
>> - MXR_LAYER_CFG_VP_MASK);
>> + struct exynos_drm_plane_state *state;
>> + struct drm_framebuffer *fb;
>> + unsigned int priority;
>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
>> + bool enable;
>> +
>> + for (win = 0; win < MIXER_WIN_NR; ++win) {
>> + state = to_exynos_plane_state(ctx->planes[win].base.state);
>> + fb = state->fb;
>> +
>> + priority = state->base.normalized_zpos + 1;
>> + enable = test_bit(win, &ctx->state_cache);
>> +
>> + if (!enable)
>> + continue;
>> +
>> + switch (win) {
>> + case 0:
>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE;
>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);
>> + break;
>> +
>> + case 1:
>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE;
>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);
>> + break;
>> +
>> + case VP_DEFAULT_WIN:
>> + vp_enable = VP_ENABLE_ON;
>> + mxr_cfg |= MXR_CFG_VP_ENABLE;
>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);
>> + break;
>> + }
>> +
>> + if (!fb)
>> + continue;
>> +
>> + /*
>> + * TODO: Don't enable alpha blending for the bottom window.
>> + */
>> + switch (win) {
>> + case 0:
>> + case 1:
>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>> + break;
>> +
>> + case VP_DEFAULT_WIN:
>> + mixer_cfg_vp_blend(ctx);
>> + break;
>> }
>> - break;
>> }
>> +
>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);
>> +
>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
>
> What about enabled planes which are not updated?
> Corresponding bit in ctx->state_cache will be 0.
Hmm, it shouldn't. state_cache is initialized once when the mixer
context struct is calloced. If the plane is not updated, the
corresponding bit in state_cache doesn't change, hence it stays 'on' in
this case (enabled plane).
But I guess one could make this more explicit, by putting state_cache
initialisation in mixer_win_reset() and replace the manipulation of the
MXR_LAYER_CFG and MXR_CFG registers with a call to mixer_cfg_layer().
With best wishes,
Tobias
> It looks like this routine will disable them in hardware, am I right?
>
> Regards
> Andrzej
>
>> }
>>
>> static void mixer_run(struct mixer_context *ctx)
>> @@ -478,7 +510,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
>> struct mixer_resources *res = &ctx->mixer_res;
>> struct drm_framebuffer *fb = state->fb;
>> - unsigned int priority = state->base.normalized_zpos + 1;
>> unsigned long flags;
>> dma_addr_t luma_addr[2], chroma_addr[2];
>> bool tiled_mode = false;
>> @@ -563,8 +594,6 @@ static void vp_video_buffer(struct mixer_context *ctx,
>>
>> mixer_cfg_scan(ctx, mode->vdisplay);
>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
>> - mixer_cfg_layer(ctx, plane->index, priority, true);
>> - mixer_cfg_vp_blend(ctx);
>> mixer_run(ctx);
>>
>> spin_unlock_irqrestore(&res->reg_slock, flags);
>> @@ -588,7 +617,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>> struct drm_display_mode *mode = &state->base.crtc->state->adjusted_mode;
>> struct mixer_resources *res = &ctx->mixer_res;
>> struct drm_framebuffer *fb = state->fb;
>> - unsigned int priority = state->base.normalized_zpos + 1;
>> unsigned long flags;
>> unsigned int win = plane->index;
>> unsigned int x_ratio = 0, y_ratio = 0;
>> @@ -680,8 +708,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>>
>> mixer_cfg_scan(ctx, mode->vdisplay);
>> mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
>> - mixer_cfg_layer(ctx, win, priority, true);
>> - mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>>
>> /* layer update mandatory for mixer 16.0.33.0 */
>> if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
>> @@ -994,32 +1020,36 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
>> vp_video_buffer(mixer_ctx, plane);
>> else
>> mixer_graph_buffer(mixer_ctx, plane);
>> +
>> + __set_bit(plane->index, &mixer_ctx->state_cache);
>> }
>>
>> static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
>> struct exynos_drm_plane *plane)
>> {
>> struct mixer_context *mixer_ctx = crtc->ctx;
>> - struct mixer_resources *res = &mixer_ctx->mixer_res;
>> - unsigned long flags;
>>
>> DRM_DEBUG_KMS("win: %d\n", plane->index);
>>
>> if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>> return;
>>
>> - spin_lock_irqsave(&res->reg_slock, flags);
>> - mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
>> - spin_unlock_irqrestore(&res->reg_slock, flags);
>> + __clear_bit(plane->index, &mixer_ctx->state_cache);
>> }
>>
>> static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
>> {
>> struct mixer_context *mixer_ctx = crtc->ctx;
>> + struct mixer_resources *res = &mixer_ctx->mixer_res;
>> + unsigned long flags;
>>
>> if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>> return;
>>
>> + spin_lock_irqsave(&res->reg_slock, flags);
>> + mixer_cfg_layer(mixer_ctx);
>> + spin_unlock_irqrestore(&res->reg_slock, flags);
>> +
>> mixer_vsync_set_update(mixer_ctx, true);
>> }
>>
>> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
>> index 7f22df5..728a18e 100644
>> --- a/drivers/gpu/drm/exynos/regs-mixer.h
>> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
>> @@ -100,6 +100,7 @@
>> #define MXR_CFG_GRP1_ENABLE (1 << 5)
>> #define MXR_CFG_GRP0_ENABLE (1 << 4)
>> #define MXR_CFG_VP_ENABLE (1 << 3)
>> +#define MXR_CFG_ENABLE_MASK (0x7 << 3)
>> #define MXR_CFG_SCAN_INTERLACE (0 << 2)
>> #define MXR_CFG_SCAN_PROGRESSIVE (1 << 2)
>> #define MXR_CFG_SCAN_NTSC (0 << 1)
>> @@ -151,6 +152,7 @@
>> #define MXR_LAYER_CFG_GRP0_MASK MXR_LAYER_CFG_GRP0_VAL(~0)
>> #define MXR_LAYER_CFG_VP_VAL(x) MXR_MASK_VAL(x, 3, 0)
>> #define MXR_LAYER_CFG_VP_MASK MXR_LAYER_CFG_VP_VAL(~0)
>> +#define MXR_LAYER_CFG_MASK 0xFFF
>>
>> #endif /* SAMSUNG_REGS_MIXER_H */
>>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
2016-09-20 11:23 ` Tobias Jakobi
@ 2016-09-20 12:34 ` Andrzej Hajda
2016-09-20 12:50 ` Andrzej Hajda
0 siblings, 1 reply; 7+ messages in thread
From: Andrzej Hajda @ 2016-09-20 12:34 UTC (permalink / raw)
To: Tobias Jakobi, linux-samsung-soc; +Cc: dri-devel, m.szyprowski
On 20.09.2016 13:23, Tobias Jakobi wrote:
> Hello Andrzej,
>
> first of all, I've noticed an error myself. mixer_disable() calls
> mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
>
> Now to your points...
>
>
> Andrzej Hajda wrote:
>> On 19.09.2016 16:16, Tobias Jakobi wrote:
>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once
>>> in mixer_cfg_layer().
>>> Trigger this via atomic flush.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>> drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------
>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 +
>>> 2 files changed, 69 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 1e78d57..d4efd9c 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -99,6 +99,7 @@ struct mixer_context {
>>> struct drm_device *drm_dev;
>>> struct exynos_drm_crtc *crtc;
>>> struct exynos_drm_plane planes[MIXER_WIN_NR];
>>> + unsigned long state_cache;
>> The name of the variable is cryptic.
> Yes, I'll try to come up with something better. It would probably be
> easier if struct mixer_context had a documentation for its fields.
>
> Anyway, any suggestions?
(active|enabled)_(planes|windows), or sth similar?
>
>
>>> int pipe;
>>> unsigned long flags;
>>>
>>> @@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
>>> }
>>>
>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>> - unsigned int priority, bool enable)
>>> +static void mixer_cfg_layer(struct mixer_context *ctx)
>>> {
>>> struct mixer_resources *res = &ctx->mixer_res;
>>> - u32 val = enable ? ~0 : 0;
>>> -
>>> - switch (win) {
>>> - case 0:
>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>> - MXR_LAYER_CFG_GRP0_VAL(priority),
>>> - MXR_LAYER_CFG_GRP0_MASK);
>>> - break;
>>> - case 1:
>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>> - MXR_LAYER_CFG_GRP1_VAL(priority),
>>> - MXR_LAYER_CFG_GRP1_MASK);
>>> + unsigned int win;
>>>
>>> - break;
>>> - case VP_DEFAULT_WIN:
>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
>>> - mixer_reg_writemask(res, MXR_CFG, val,
>>> - MXR_CFG_VP_ENABLE);
>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>> - MXR_LAYER_CFG_VP_VAL(priority),
>>> - MXR_LAYER_CFG_VP_MASK);
>>> + struct exynos_drm_plane_state *state;
>>> + struct drm_framebuffer *fb;
>>> + unsigned int priority;
>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
>>> + bool enable;
>>> +
>>> + for (win = 0; win < MIXER_WIN_NR; ++win) {
>>> + state = to_exynos_plane_state(ctx->planes[win].base.state);
>>> + fb = state->fb;
>>> +
>>> + priority = state->base.normalized_zpos + 1;
>>> + enable = test_bit(win, &ctx->state_cache);
>>> +
>>> + if (!enable)
>>> + continue;
>>> +
>>> + switch (win) {
>>> + case 0:
>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE;
>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);
>>> + break;
>>> +
>>> + case 1:
>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE;
>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);
>>> + break;
>>> +
>>> + case VP_DEFAULT_WIN:
>>> + vp_enable = VP_ENABLE_ON;
>>> + mxr_cfg |= MXR_CFG_VP_ENABLE;
>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);
>>> + break;
>>> + }
>>> +
>>> + if (!fb)
>>> + continue;
>>> +
>>> + /*
>>> + * TODO: Don't enable alpha blending for the bottom window.
>>> + */
>>> + switch (win) {
>>> + case 0:
>>> + case 1:
>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>>> + break;
>>> +
>>> + case VP_DEFAULT_WIN:
>>> + mixer_cfg_vp_blend(ctx);
>>> + break;
>>> }
>>> - break;
>>> }
>>> +
>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);
>>> +
>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
>> What about enabled planes which are not updated?
>> Corresponding bit in ctx->state_cache will be 0.
> Hmm, it shouldn't. state_cache is initialized once when the mixer
> context struct is calloced. If the plane is not updated, the
> corresponding bit in state_cache doesn't change, hence it stays 'on' in
> this case (enabled plane).
Thanks for the explanation, I have though (incorrectly) that state_cache is
reset before every atomic transaction.
By the way I wonder, if we cannot get information if window is enabled
by checking 'plane_state->crtc && plane_state->crtc->state->active'.
>
> But I guess one could make this more explicit, by putting state_cache
> initialisation in mixer_win_reset() and replace the manipulation of the
> MXR_LAYER_CFG and MXR_CFG registers with a call to mixer_cfg_layer().
I think now it is OK, just good name for the variable should be enough
to make it more readable.
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
2016-09-20 12:34 ` Andrzej Hajda
@ 2016-09-20 12:50 ` Andrzej Hajda
2016-09-20 14:09 ` Tobias Jakobi
0 siblings, 1 reply; 7+ messages in thread
From: Andrzej Hajda @ 2016-09-20 12:50 UTC (permalink / raw)
To: Tobias Jakobi, linux-samsung-soc; +Cc: dri-devel, m.szyprowski
On 20.09.2016 14:34, Andrzej Hajda wrote:
> On 20.09.2016 13:23, Tobias Jakobi wrote:
>> Hello Andrzej,
>>
>> first of all, I've noticed an error myself. mixer_disable() calls
>> mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
>>
>> Now to your points...
>>
>>
>> Andrzej Hajda wrote:
>>> On 19.09.2016 16:16, Tobias Jakobi wrote:
>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once
>>>> in mixer_cfg_layer().
>>>> Trigger this via atomic flush.
>>>>
>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>> ---
>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------
>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 +
>>>> 2 files changed, 69 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> index 1e78d57..d4efd9c 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> @@ -99,6 +99,7 @@ struct mixer_context {
>>>> struct drm_device *drm_dev;
>>>> struct exynos_drm_crtc *crtc;
>>>> struct exynos_drm_plane planes[MIXER_WIN_NR];
>>>> + unsigned long state_cache;
>>> The name of the variable is cryptic.
>> Yes, I'll try to come up with something better. It would probably be
>> easier if struct mixer_context had a documentation for its fields.
>>
>> Anyway, any suggestions?
> (active|enabled)_(planes|windows), or sth similar?
>
>>
>>>> int pipe;
>>>> unsigned long flags;
>>>>
>>>> @@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
>>>> }
>>>>
>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>> - unsigned int priority, bool enable)
>>>> +static void mixer_cfg_layer(struct mixer_context *ctx)
>>>> {
>>>> struct mixer_resources *res = &ctx->mixer_res;
>>>> - u32 val = enable ? ~0 : 0;
>>>> -
>>>> - switch (win) {
>>>> - case 0:
>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>> - MXR_LAYER_CFG_GRP0_VAL(priority),
>>>> - MXR_LAYER_CFG_GRP0_MASK);
>>>> - break;
>>>> - case 1:
>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>> - MXR_LAYER_CFG_GRP1_VAL(priority),
>>>> - MXR_LAYER_CFG_GRP1_MASK);
>>>> + unsigned int win;
>>>>
>>>> - break;
>>>> - case VP_DEFAULT_WIN:
>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
>>>> - mixer_reg_writemask(res, MXR_CFG, val,
>>>> - MXR_CFG_VP_ENABLE);
>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>> - MXR_LAYER_CFG_VP_VAL(priority),
>>>> - MXR_LAYER_CFG_VP_MASK);
>>>> + struct exynos_drm_plane_state *state;
>>>> + struct drm_framebuffer *fb;
>>>> + unsigned int priority;
>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
>>>> + bool enable;
>>>> +
>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) {
>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state);
>>>> + fb = state->fb;
>>>> +
>>>> + priority = state->base.normalized_zpos + 1;
>>>> + enable = test_bit(win, &ctx->state_cache);
>>>> +
>>>> + if (!enable)
>>>> + continue;
>>>> +
>>>> + switch (win) {
>>>> + case 0:
>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE;
>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);
>>>> + break;
>>>> +
>>>> + case 1:
>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE;
>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);
>>>> + break;
>>>> +
>>>> + case VP_DEFAULT_WIN:
>>>> + vp_enable = VP_ENABLE_ON;
>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE;
>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);
>>>> + break;
>>>> + }
>>>> +
>>>> + if (!fb)
>>>> + continue;
>>>> +
>>>> + /*
>>>> + * TODO: Don't enable alpha blending for the bottom window.
>>>> + */
>>>> + switch (win) {
>>>> + case 0:
>>>> + case 1:
>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>>>> + break;
>>>> +
>>>> + case VP_DEFAULT_WIN:
>>>> + mixer_cfg_vp_blend(ctx);
>>>> + break;
>>>> }
>>>> - break;
>>>> }
>>>> +
>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);
>>>> +
>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
>>> What about enabled planes which are not updated?
>>> Corresponding bit in ctx->state_cache will be 0.
>> Hmm, it shouldn't. state_cache is initialized once when the mixer
>> context struct is calloced. If the plane is not updated, the
>> corresponding bit in state_cache doesn't change, hence it stays 'on' in
>> this case (enabled plane).
> Thanks for the explanation, I have though (incorrectly) that state_cache is
> reset before every atomic transaction.
> By the way I wonder, if we cannot get information if window is enabled
> by checking 'plane_state->crtc && plane_state->crtc->state->active'.
drm_atomic_crtc_for_each_plane seems to be a better choice,
msm and vc4 uses it in flush function.
Regards
Andrzej
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
2016-09-20 12:50 ` Andrzej Hajda
@ 2016-09-20 14:09 ` Tobias Jakobi
2016-09-20 14:30 ` Andrzej Hajda
0 siblings, 1 reply; 7+ messages in thread
From: Tobias Jakobi @ 2016-09-20 14:09 UTC (permalink / raw)
To: Andrzej Hajda, linux-samsung-soc; +Cc: dri-devel, m.szyprowski
Hi Andrzej,
Andrzej Hajda wrote:
> On 20.09.2016 14:34, Andrzej Hajda wrote:
>> On 20.09.2016 13:23, Tobias Jakobi wrote:
>>> Hello Andrzej,
>>>
>>> first of all, I've noticed an error myself. mixer_disable() calls
>>> mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
>>>
>>> Now to your points...
>>>
>>>
>>> Andrzej Hajda wrote:
>>>> On 19.09.2016 16:16, Tobias Jakobi wrote:
>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once
>>>>> in mixer_cfg_layer().
>>>>> Trigger this via atomic flush.
>>>>>
>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>>> ---
>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------
>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 +
>>>>> 2 files changed, 69 insertions(+), 37 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>> index 1e78d57..d4efd9c 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>> @@ -99,6 +99,7 @@ struct mixer_context {
>>>>> struct drm_device *drm_dev;
>>>>> struct exynos_drm_crtc *crtc;
>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR];
>>>>> + unsigned long state_cache;
>>>> The name of the variable is cryptic.
>>> Yes, I'll try to come up with something better. It would probably be
>>> easier if struct mixer_context had a documentation for its fields.
>>>
>>> Anyway, any suggestions?
>> (active|enabled)_(planes|windows), or sth similar?
Thanks, I think I'll go with the 'window' terminology then.
>>>>> int pipe;
>>>>> unsigned long flags;
>>>>>
>>>>> @@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
>>>>> }
>>>>>
>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>>> - unsigned int priority, bool enable)
>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx)
>>>>> {
>>>>> struct mixer_resources *res = &ctx->mixer_res;
>>>>> - u32 val = enable ? ~0 : 0;
>>>>> -
>>>>> - switch (win) {
>>>>> - case 0:
>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority),
>>>>> - MXR_LAYER_CFG_GRP0_MASK);
>>>>> - break;
>>>>> - case 1:
>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority),
>>>>> - MXR_LAYER_CFG_GRP1_MASK);
>>>>> + unsigned int win;
>>>>>
>>>>> - break;
>>>>> - case VP_DEFAULT_WIN:
>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
>>>>> - mixer_reg_writemask(res, MXR_CFG, val,
>>>>> - MXR_CFG_VP_ENABLE);
>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>>> - MXR_LAYER_CFG_VP_VAL(priority),
>>>>> - MXR_LAYER_CFG_VP_MASK);
>>>>> + struct exynos_drm_plane_state *state;
>>>>> + struct drm_framebuffer *fb;
>>>>> + unsigned int priority;
>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
>>>>> + bool enable;
>>>>> +
>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) {
>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state);
>>>>> + fb = state->fb;
>>>>> +
>>>>> + priority = state->base.normalized_zpos + 1;
>>>>> + enable = test_bit(win, &ctx->state_cache);
>>>>> +
>>>>> + if (!enable)
>>>>> + continue;
>>>>> +
>>>>> + switch (win) {
>>>>> + case 0:
>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE;
>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);
>>>>> + break;
>>>>> +
>>>>> + case 1:
>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE;
>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);
>>>>> + break;
>>>>> +
>>>>> + case VP_DEFAULT_WIN:
>>>>> + vp_enable = VP_ENABLE_ON;
>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE;
>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + if (!fb)
>>>>> + continue;
>>>>> +
>>>>> + /*
>>>>> + * TODO: Don't enable alpha blending for the bottom window.
>>>>> + */
>>>>> + switch (win) {
>>>>> + case 0:
>>>>> + case 1:
>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>>>>> + break;
>>>>> +
>>>>> + case VP_DEFAULT_WIN:
>>>>> + mixer_cfg_vp_blend(ctx);
>>>>> + break;
>>>>> }
>>>>> - break;
>>>>> }
>>>>> +
>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);
>>>>> +
>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
>>>> What about enabled planes which are not updated?
>>>> Corresponding bit in ctx->state_cache will be 0.
>>> Hmm, it shouldn't. state_cache is initialized once when the mixer
>>> context struct is calloced. If the plane is not updated, the
>>> corresponding bit in state_cache doesn't change, hence it stays 'on' in
>>> this case (enabled plane).
>> Thanks for the explanation, I have though (incorrectly) that state_cache is
>> reset before every atomic transaction.
>> By the way I wonder, if we cannot get information if window is enabled
>> by checking 'plane_state->crtc && plane_state->crtc->state->active'.
>
> drm_atomic_crtc_for_each_plane seems to be a better choice,
> msm and vc4 uses it in flush function.
I have looked into this, but this seems to be quite a detour.
If I understand atomic sequencing correctly, then we always have:
(1) atomic_begin()
(2) calls to update_plane() and disable_plane()
(3) atomic_flush()
We already get all the necessary information through the calls in step
(2), so I don't see the need to query this information again from DRM
core in step (3). Especially since this means iterating over lists again.
In particular drm_atomic_crtc_for_each_plane() does list_for_each_entry
calling drm_plane_index() in each step, which in turn iterates over the
list of planes again.
I rather put another unsigned long into the mixer context and walk a
normal array in mixer_cfg_layer(). :-)
With best wishes,
Tobias
>
> Regards
> Andrzej
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush()
2016-09-20 14:09 ` Tobias Jakobi
@ 2016-09-20 14:30 ` Andrzej Hajda
0 siblings, 0 replies; 7+ messages in thread
From: Andrzej Hajda @ 2016-09-20 14:30 UTC (permalink / raw)
To: Tobias Jakobi, linux-samsung-soc; +Cc: dri-devel, m.szyprowski
On 20.09.2016 16:09, Tobias Jakobi wrote:
> Hi Andrzej,
>
>
> Andrzej Hajda wrote:
>> On 20.09.2016 14:34, Andrzej Hajda wrote:
>>> On 20.09.2016 13:23, Tobias Jakobi wrote:
>>>> Hello Andrzej,
>>>>
>>>> first of all, I've noticed an error myself. mixer_disable() calls
>>>> mixer_disable_plane(), so it should also be modified. I'll send a v2 later.
>>>>
>>>> Now to your points...
>>>>
>>>>
>>>> Andrzej Hajda wrote:
>>>>> On 19.09.2016 16:16, Tobias Jakobi wrote:
>>>>>> Only manipulate the MXR_CFG and MXR_LAYER_CFG registers once
>>>>>> in mixer_cfg_layer().
>>>>>> Trigger this via atomic flush.
>>>>>>
>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>>>> ---
>>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 104 ++++++++++++++++++++++------------
>>>>>> drivers/gpu/drm/exynos/regs-mixer.h | 2 +
>>>>>> 2 files changed, 69 insertions(+), 37 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>>> index 1e78d57..d4efd9c 100644
>>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>>> @@ -99,6 +99,7 @@ struct mixer_context {
>>>>>> struct drm_device *drm_dev;
>>>>>> struct exynos_drm_crtc *crtc;
>>>>>> struct exynos_drm_plane planes[MIXER_WIN_NR];
>>>>>> + unsigned long state_cache;
>>>>> The name of the variable is cryptic.
>>>> Yes, I'll try to come up with something better. It would probably be
>>>> easier if struct mixer_context had a documentation for its fields.
>>>>
>>>> Anyway, any suggestions?
>>> (active|enabled)_(planes|windows), or sth similar?
> Thanks, I think I'll go with the 'window' terminology then.
>
>
>>>>>> int pipe;
>>>>>> unsigned long flags;
>>>>>>
>>>>>> @@ -418,37 +419,68 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
>>>>>> mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_RGB_FMT_MASK);
>>>>>> }
>>>>>>
>>>>>> -static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>>>> - unsigned int priority, bool enable)
>>>>>> +static void mixer_cfg_layer(struct mixer_context *ctx)
>>>>>> {
>>>>>> struct mixer_resources *res = &ctx->mixer_res;
>>>>>> - u32 val = enable ? ~0 : 0;
>>>>>> -
>>>>>> - switch (win) {
>>>>>> - case 0:
>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>>>> - MXR_LAYER_CFG_GRP0_VAL(priority),
>>>>>> - MXR_LAYER_CFG_GRP0_MASK);
>>>>>> - break;
>>>>>> - case 1:
>>>>>> - mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>>>> - MXR_LAYER_CFG_GRP1_VAL(priority),
>>>>>> - MXR_LAYER_CFG_GRP1_MASK);
>>>>>> + unsigned int win;
>>>>>>
>>>>>> - break;
>>>>>> - case VP_DEFAULT_WIN:
>>>>>> - if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags)) {
>>>>>> - vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
>>>>>> - mixer_reg_writemask(res, MXR_CFG, val,
>>>>>> - MXR_CFG_VP_ENABLE);
>>>>>> - mixer_reg_writemask(res, MXR_LAYER_CFG,
>>>>>> - MXR_LAYER_CFG_VP_VAL(priority),
>>>>>> - MXR_LAYER_CFG_VP_MASK);
>>>>>> + struct exynos_drm_plane_state *state;
>>>>>> + struct drm_framebuffer *fb;
>>>>>> + unsigned int priority;
>>>>>> + u32 mxr_cfg = 0, mxr_layer_cfg = 0, vp_enable = 0;
>>>>>> + bool enable;
>>>>>> +
>>>>>> + for (win = 0; win < MIXER_WIN_NR; ++win) {
>>>>>> + state = to_exynos_plane_state(ctx->planes[win].base.state);
>>>>>> + fb = state->fb;
>>>>>> +
>>>>>> + priority = state->base.normalized_zpos + 1;
>>>>>> + enable = test_bit(win, &ctx->state_cache);
>>>>>> +
>>>>>> + if (!enable)
>>>>>> + continue;
>>>>>> +
>>>>>> + switch (win) {
>>>>>> + case 0:
>>>>>> + mxr_cfg |= MXR_CFG_GRP0_ENABLE;
>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP0_VAL(priority);
>>>>>> + break;
>>>>>> +
>>>>>> + case 1:
>>>>>> + mxr_cfg |= MXR_CFG_GRP1_ENABLE;
>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_GRP1_VAL(priority);
>>>>>> + break;
>>>>>> +
>>>>>> + case VP_DEFAULT_WIN:
>>>>>> + vp_enable = VP_ENABLE_ON;
>>>>>> + mxr_cfg |= MXR_CFG_VP_ENABLE;
>>>>>> + mxr_layer_cfg |= MXR_LAYER_CFG_VP_VAL(priority);
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + if (!fb)
>>>>>> + continue;
>>>>>> +
>>>>>> + /*
>>>>>> + * TODO: Don't enable alpha blending for the bottom window.
>>>>>> + */
>>>>>> + switch (win) {
>>>>>> + case 0:
>>>>>> + case 1:
>>>>>> + mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>>>>>> + break;
>>>>>> +
>>>>>> + case VP_DEFAULT_WIN:
>>>>>> + mixer_cfg_vp_blend(ctx);
>>>>>> + break;
>>>>>> }
>>>>>> - break;
>>>>>> }
>>>>>> +
>>>>>> + if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
>>>>>> + vp_reg_writemask(res, VP_ENABLE, vp_enable, VP_ENABLE_ON);
>>>>>> +
>>>>>> + mixer_reg_writemask(res, MXR_CFG, mxr_cfg, MXR_CFG_ENABLE_MASK);
>>>>>> + mixer_reg_writemask(res, MXR_LAYER_CFG, mxr_layer_cfg, MXR_LAYER_CFG_MASK);
>>>>> What about enabled planes which are not updated?
>>>>> Corresponding bit in ctx->state_cache will be 0.
>>>> Hmm, it shouldn't. state_cache is initialized once when the mixer
>>>> context struct is calloced. If the plane is not updated, the
>>>> corresponding bit in state_cache doesn't change, hence it stays 'on' in
>>>> this case (enabled plane).
>>> Thanks for the explanation, I have though (incorrectly) that state_cache is
>>> reset before every atomic transaction.
>>> By the way I wonder, if we cannot get information if window is enabled
>>> by checking 'plane_state->crtc && plane_state->crtc->state->active'.
>> drm_atomic_crtc_for_each_plane seems to be a better choice,
>> msm and vc4 uses it in flush function.
> I have looked into this, but this seems to be quite a detour.
>
> If I understand atomic sequencing correctly, then we always have:
> (1) atomic_begin()
> (2) calls to update_plane() and disable_plane()
> (3) atomic_flush()
>
> We already get all the necessary information through the calls in step
> (2), so I don't see the need to query this information again from DRM
> core in step (3). Especially since this means iterating over lists again.
>
> In particular drm_atomic_crtc_for_each_plane() does list_for_each_entry
> calling drm_plane_index() in each step, which in turn iterates over the
> list of planes again.
Fortunately drm_plane_index() is saner since 'drm: Store the plane's index'
patch, so the loop have similar complexity, but is more generic.
Moreover state_cache duplicates drm_crtc_state::plane_mask.
Anyway I have no strong feeling about it, do as you wish :)
Regards
Andrzej
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-20 14:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20160919141707eucas1p281593c95940e8c0b833a84175f39ffe2@eucas1p2.samsung.com>
2016-09-19 14:16 ` [RFC 2/2] drm/exynos: mixer: configure layers once in mixer_atomic_flush() Tobias Jakobi
2016-09-20 10:00 ` Andrzej Hajda
2016-09-20 11:23 ` Tobias Jakobi
2016-09-20 12:34 ` Andrzej Hajda
2016-09-20 12:50 ` Andrzej Hajda
2016-09-20 14:09 ` Tobias Jakobi
2016-09-20 14:30 ` Andrzej Hajda
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.