Re: [PATCH 01/10] drm/exynos/mixer: abstract out output mode setup code

[Andrzej Hajda]

Hi Tobias,

Thanks for review.


On 12.09.2017 14:29, Tobias Jakobi wrote:
> Hello Andrzej,
>
>
> Andrzej Hajda wrote:
>> Mode setup code is called from video plane update and mixer plane update.
>> Lets group it together in mixer_commit function like in case of other
>> Exynos CRTCs.
> Minor typo: Lets --> Let's
>
> Reviewed-by: Tobias Jakobi 
>
> With some small question below.
>
>
>> Signed-off-by: Andrzej Hajda 
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 33 ++---
>>  1 file changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
>> b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 0027554..499ebdc 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -473,6 +473,22 @@ static void mixer_stop(struct mixer_context *ctx)
>>  usleep_range(1, 12000);
>>  }
>>  
>> +static void mixer_commit(struct mixer_context *ctx)
>> +{
>> +struct drm_display_mode *mode = >crtc->base.state->adjusted_mode;
>> +
>> +/* setup display size */
>> +if (ctx->mxr_ver == MXR_VER_128_0_0_184) {
>> +u32 val  = MXR_MXR_RES_HEIGHT(mode->vdisplay)
>> + | MXR_MXR_RES_WIDTH(mode->hdisplay);
>> +mixer_reg_write(>mixer_res, MXR_RESOLUTION, val);
>> +}
>> +
>> +mixer_cfg_scan(ctx, mode->vdisplay);
>> +mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
>> +mixer_run(ctx);
>> +}
>> +
>>  static void vp_video_buffer(struct mixer_context *ctx,
>>  struct exynos_drm_plane *plane)
>>  {
>> @@ -553,11 +569,9 @@ static void vp_video_buffer(struct mixer_context *ctx,
>>  vp_reg_write(res, VP_TOP_C_PTR, chroma_addr[0]);
>>  vp_reg_write(res, VP_BOT_C_PTR, chroma_addr[1]);
>>  
>> -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);
>> +mixer_commit(ctx);
>>  
>>  spin_unlock_irqrestore(>reg_slock, flags);
>>  
>> @@ -638,14 +652,6 @@ static void mixer_graph_buffer(struct mixer_context 
>> *ctx,
>>  mixer_reg_write(res, MXR_GRAPHIC_SPAN(win),
>>  fb->pitches[0] / fb->format->cpp[0]);
>>  
>> -/* setup display size */
>> -if (ctx->mxr_ver == MXR_VER_128_0_0_184 &&
>> -win == DEFAULT_WIN) {
>> -val  = MXR_MXR_RES_HEIGHT(mode->vdisplay);
>> -val |= MXR_MXR_RES_WIDTH(mode->hdisplay);
>> -mixer_reg_write(res, MXR_RESOLUTION, val);
>> -}
>> -
>>  val  = MXR_GRP_WH_WIDTH(state->src.w);
>>  val |= MXR_GRP_WH_HEIGHT(state->src.h);
>>  val |= MXR_GRP_WH_H_SCALE(x_ratio);
>> @@ -660,18 +666,15 @@ static void mixer_graph_buffer(struct mixer_context 
>> *ctx,
>>  /* set buffer address to mixer */
>>  mixer_reg_write(res, MXR_GRAPHIC_BASE(win), dma_addr);
>>  
>> -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->format->format));
>> +mixer_commit(ctx);
> With this change, mixer_run() is now done before mixer_layer_update(). I just
> wanted to make sure that the order of operations isn't critical here.

It shoudn't matter, as it is between atomic_begin, which disables shadow
updates,
and atomic_flush, which re-enables them.

Regards
Andrzej

>
>
>
>>  /* layer update mandatory for mixer 16.0.33.0 */
>>  if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
>>  ctx->mxr_ver == MXR_VER_128_0_0_184)
>>  mixer_layer_update(ctx);
>>  
>> -mixer_run(ctx);
>> -
>>  spin_unlock_irqrestore(>reg_slock, flags);
>>  
>>  mixer_regs_dump(ctx);
>>
>
>
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm/exynos/mixer: abstract out output mode setup code

[Tobias Jakobi]

Hello Andrzej,


Andrzej Hajda wrote:
> Mode setup code is called from video plane update and mixer plane update.
> Lets group it together in mixer_commit function like in case of other
> Exynos CRTCs.
Minor typo: Lets --> Let's

Reviewed-by: Tobias Jakobi 

With some small question below.


> Signed-off-by: Andrzej Hajda 
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 33 ++---
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
> b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 0027554..499ebdc 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -473,6 +473,22 @@ static void mixer_stop(struct mixer_context *ctx)
>   usleep_range(1, 12000);
>  }
>  
> +static void mixer_commit(struct mixer_context *ctx)
> +{
> + struct drm_display_mode *mode = >crtc->base.state->adjusted_mode;
> +
> + /* setup display size */
> + if (ctx->mxr_ver == MXR_VER_128_0_0_184) {
> + u32 val  = MXR_MXR_RES_HEIGHT(mode->vdisplay)
> +  | MXR_MXR_RES_WIDTH(mode->hdisplay);
> + mixer_reg_write(>mixer_res, MXR_RESOLUTION, val);
> + }
> +
> + mixer_cfg_scan(ctx, mode->vdisplay);
> + mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
> + mixer_run(ctx);
> +}
> +
>  static void vp_video_buffer(struct mixer_context *ctx,
>   struct exynos_drm_plane *plane)
>  {
> @@ -553,11 +569,9 @@ static void vp_video_buffer(struct mixer_context *ctx,
>   vp_reg_write(res, VP_TOP_C_PTR, chroma_addr[0]);
>   vp_reg_write(res, VP_BOT_C_PTR, chroma_addr[1]);
>  
> - 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);
> + mixer_commit(ctx);
>  
>   spin_unlock_irqrestore(>reg_slock, flags);
>  
> @@ -638,14 +652,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>   mixer_reg_write(res, MXR_GRAPHIC_SPAN(win),
>   fb->pitches[0] / fb->format->cpp[0]);
>  
> - /* setup display size */
> - if (ctx->mxr_ver == MXR_VER_128_0_0_184 &&
> - win == DEFAULT_WIN) {
> - val  = MXR_MXR_RES_HEIGHT(mode->vdisplay);
> - val |= MXR_MXR_RES_WIDTH(mode->hdisplay);
> - mixer_reg_write(res, MXR_RESOLUTION, val);
> - }
> -
>   val  = MXR_GRP_WH_WIDTH(state->src.w);
>   val |= MXR_GRP_WH_HEIGHT(state->src.h);
>   val |= MXR_GRP_WH_H_SCALE(x_ratio);
> @@ -660,18 +666,15 @@ static void mixer_graph_buffer(struct mixer_context 
> *ctx,
>   /* set buffer address to mixer */
>   mixer_reg_write(res, MXR_GRAPHIC_BASE(win), dma_addr);
>  
> - 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->format->format));
> + mixer_commit(ctx);
With this change, mixer_run() is now done before mixer_layer_update(). I just
wanted to make sure that the order of operations isn't critical here.



>   /* layer update mandatory for mixer 16.0.33.0 */
>   if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
>   ctx->mxr_ver == MXR_VER_128_0_0_184)
>   mixer_layer_update(ctx);
>  
> - mixer_run(ctx);
> -
>   spin_unlock_irqrestore(>reg_slock, flags);
>  
>   mixer_regs_dump(ctx);
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/10] drm/exynos/mixer: abstract out output mode setup code

[Andrzej Hajda]

Mode setup code is called from video plane update and mixer plane update.
Lets group it together in mixer_commit function like in case of other
Exynos CRTCs.

Signed-off-by: Andrzej Hajda 
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 33 ++---
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
b/drivers/gpu/drm/exynos/exynos_mixer.c
index 0027554..499ebdc 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -473,6 +473,22 @@ static void mixer_stop(struct mixer_context *ctx)
usleep_range(1, 12000);
 }
 
+static void mixer_commit(struct mixer_context *ctx)
+{
+   struct drm_display_mode *mode = >crtc->base.state->adjusted_mode;
+
+   /* setup display size */
+   if (ctx->mxr_ver == MXR_VER_128_0_0_184) {
+   u32 val  = MXR_MXR_RES_HEIGHT(mode->vdisplay)
+| MXR_MXR_RES_WIDTH(mode->hdisplay);
+   mixer_reg_write(>mixer_res, MXR_RESOLUTION, val);
+   }
+
+   mixer_cfg_scan(ctx, mode->vdisplay);
+   mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
+   mixer_run(ctx);
+}
+
 static void vp_video_buffer(struct mixer_context *ctx,
struct exynos_drm_plane *plane)
 {
@@ -553,11 +569,9 @@ static void vp_video_buffer(struct mixer_context *ctx,
vp_reg_write(res, VP_TOP_C_PTR, chroma_addr[0]);
vp_reg_write(res, VP_BOT_C_PTR, chroma_addr[1]);
 
-   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);
+   mixer_commit(ctx);
 
spin_unlock_irqrestore(>reg_slock, flags);
 
@@ -638,14 +652,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
mixer_reg_write(res, MXR_GRAPHIC_SPAN(win),
fb->pitches[0] / fb->format->cpp[0]);
 
-   /* setup display size */
-   if (ctx->mxr_ver == MXR_VER_128_0_0_184 &&
-   win == DEFAULT_WIN) {
-   val  = MXR_MXR_RES_HEIGHT(mode->vdisplay);
-   val |= MXR_MXR_RES_WIDTH(mode->hdisplay);
-   mixer_reg_write(res, MXR_RESOLUTION, val);
-   }
-
val  = MXR_GRP_WH_WIDTH(state->src.w);
val |= MXR_GRP_WH_HEIGHT(state->src.h);
val |= MXR_GRP_WH_H_SCALE(x_ratio);
@@ -660,18 +666,15 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
/* set buffer address to mixer */
mixer_reg_write(res, MXR_GRAPHIC_BASE(win), dma_addr);
 
-   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->format->format));
+   mixer_commit(ctx);
 
/* layer update mandatory for mixer 16.0.33.0 */
if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
ctx->mxr_ver == MXR_VER_128_0_0_184)
mixer_layer_update(ctx);
 
-   mixer_run(ctx);
-
spin_unlock_irqrestore(>reg_slock, flags);
 
mixer_regs_dump(ctx);
-- 
2.7.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel