Re: [PATCH v2 4/5] drm/exynos: mixer: do blending setup in mixer_cfg_layer()

[Tobias Jakobi <liquid.acid@gmx.net>]

Hello Inki,

my main system which I also used for development was stolen on Tuesday,
so I won't be working on this series anytime soon. If anyone wants to
pick it up, please go ahead.

- Tobias


Inki Dae wrote:
> 2015-11-24 2:44 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
>> Hey Inki,
>>
>>
>> Inki Dae wrote:
>>>
>>> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>>>> This updates the blending setup when the layer configuration
>>>> changes (triggered by mixer_win_{commit,disable}).
>>>>
>>>> To avoid unnecesary reconfigurations we cache the layer
>>>> state in the mixer context.
>>>>
>>>> Extra care has to be taken for the layer that is currently
>>>> being enabled/disabled.
>>>>
>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 41 +++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 39 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> index ec9659e..1c24fb5 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 exynos_drm_plane planes[MIXER_WIN_NR];
>>>>      const struct layer_cfg  *layer_cfg;
>>>>      unsigned int            num_layer;
>>>> +    u32                     layer_state;
>>>>      int                     pipe;
>>>>      unsigned long           flags;
>>>>      bool                    interlace;
>>>> @@ -189,6 +190,27 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
>>>>      }
>>>>  }
>>>>
>>>> +static inline u32 get_layer_state(const struct mixer_context *ctx,
>>>> +    unsigned int win, bool enable)
>>>> +{
>>>> +    u32 enable_state, alpha_state;
>>>> +
>>>> +    enable_state =tx->layer_state & 0xffff;
>>>> +    alpha_state =tx->layer_state >> 16;
>>>> +
>>>> +    if (enable)
>>>> +            enable_state |=1 << win);
>>>> +    else
>>>> +            enable_state &=(1 << win);
>>>> +
>>>> +    if (enable && is_alpha_format(ctx, win))
>>>> +            alpha_state |=1 << win);
>>>> +    else
>>>> +            alpha_state &=(1 << win);
>>>> +
>>>> +    return ((alpha_state << 16) | enable_state);
>>>> +}
>>>> +
>>>>  static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>>>  {
>>>>      return readl(res->vp_regs + reg_id);
>>>> @@ -370,8 +392,9 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>>>  {
>>>>      u32 val;
>>>>      struct mixer_resources *res =ctx->mixer_res;
>>>> +    const u32 alpha_state =tx->layer_state >> 16;
>>>>
>>>> -    if (is_alpha_format(ctx, cfg->index)) {
>>>> +    if (alpha_state & (1 << cfg->index)) {
>>>>              val  =XR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>>>              val |=XR_GRP_CFG_BLEND_PRE_MUL;
>>>>              val |=XR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>>>> @@ -397,10 +420,11 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>>>      }
>>>>  }
>>>>
>>>> -static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>>>> +static void mixer_layer_blending(struct mixer_context *ctx)
>>>>  {
>>>>      unsigned int i, index;
>>>>      bool bottom_layer =alse;
>>>> +    const u32 enable_state =tx->layer_state & 0xffff;
>>>>
>>>>      for (i =; i < ctx->num_layer; ++i) {
>>>>              index =tx->layer_cfg[i].index;
>>>> @@ -503,8 +527,19 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>>                              bool enable)
>>>>  {
>>>>      struct mixer_resources *res =ctx->mixer_res;
>>>> +    u32 new_layer_state;
>>>>      u32 val =nable ? ~0 : 0;
>>>>
>>>> +    new_layer_state =et_layer_state(ctx, win, enable);
>>>> +    if (new_layer_state =ctx->layer_state)
>>>> +            return;
>>>> +
>>>> +    /*
>>>> +     * Update the layer state so that mixer_layer_blending()
>>>> +     * below can use it.
>>>> +     */
>>>> +    ctx->layer_state =ew_layer_state;
>>> It may be trivial but I think it'd be better to move above line to most bottom of this function.
>> Sure, I can do that, but I would rather know what's the rationale here.
> Very simple. Above line sets layer_state to new_layer_state logically
> but physically not.
> So it'd be reasonable to update layer state after physically setting
> the mixer layer.
>
> Thanks,
> Inki Dae
>
>>
>> With best wishes,
>> Tobias
>>
>>>> +
>>>>      switch (win) {
>>>>      case 0:
>>>>              mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>>>> @@ -520,6 +555,8 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>>              }
>>>>              break;
>>>>      }
>>>> +
>>>> +    mixer_layer_blending(ctx);
>>> Here.
>>>
>>> Thanks,
>>> Inki Dae
>>>
>>>>  }
>>>>
>>>>  static void mixer_run(struct mixer_context *ctx)
>>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel