[PATCH v2 2/5] drm/exynos: mixer: introduce mixer_layer_blending()
Inki Dae
inki.dae at samsung.com
Wed Nov 25 08:31:46 PST 2015
2015-11-24 2:44 GMT+09:00 Tobias Jakobi <tjakobi at math.uni-bielefeld.de>:
> Hey Inki,
>
>
> Inki Dae wrote:
>>
>>
>> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>>> This analyses the current layer configuration (which layers
>>> are enabled, which have alpha-pixelformat, etc.) and setups
>>> blending accordingly.
>>>
>>> We currently disable all kinds of blending for the bottom-most
>>> layer, since configuration of the mixer background is not
>>> yet exposed.
>>> Also blending is only enabled when the layer has a pixelformat
>>> with alpha attached.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi at math.uni-bielefeld.de>
>>> ---
>>> drivers/gpu/drm/exynos/exynos_mixer.c | 88 +++++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/exynos/regs-mixer.h | 1 +
>>> 2 files changed, 89 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 2c1cea3..ec77aad 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -174,6 +174,21 @@ static const u8 filter_cr_horiz_tap4[] = {
>>> 70, 59, 48, 37, 27, 19, 11, 5,
>>> };
>>>
>>> +static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int win)
>>> +{
>>> + const struct drm_plane_state *state = ctx->planes[win].base.state;
>>> + const struct drm_framebuffer *fb = state->fb;
>>> +
>>> + switch (fb->pixel_format) {
>>> + case DRM_FORMAT_ARGB8888:
>>> + case DRM_FORMAT_ARGB1555:
>>> + case DRM_FORMAT_ARGB4444:
>>> + return true;
>>> + default:
>>> + return false;
>>> + }
>>> +}
>>> +
>>> static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>> {
>>> return readl(res->vp_regs + reg_id);
>>> @@ -331,6 +346,79 @@ static void mixer_layer_priority(struct mixer_context *ctx)
>>> mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
>>> }
>>>
>>> +/* Configure blending for bottom-most layer. */
>>> +static void mixer_bottom_layer(struct mixer_context *ctx,
>>> + const struct layer_cfg *cfg)
>>> +{
>>> + u32 val;
>>> + struct mixer_resources *res = &ctx->mixer_res;
>>> +
>>> + if (cfg->index == 2) {
>>> + val = 0; /* use defaults for video layer */
>>> + mixer_reg_write(res, MXR_VIDEO_CFG, val);
>>> + } else {
>>> + val = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>> +
>>> + /* Don't blend bottom-most layer onto the mixer background. */
>>> + mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>>> + val, MXR_GRP_CFG_MISC_MASK);
>>> + }
>>> +}
>>> +
>>> +static void mixer_general_layer(struct mixer_context *ctx,
>>> + const struct layer_cfg *cfg)
>>> +{
>>> + u32 val;
>>> + struct mixer_resources *res = &ctx->mixer_res;
>>> +
>>> + if (is_alpha_format(ctx, cfg->index)) {
>>> + val = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>> + val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>>> + val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>>> +
>>> + /* The video layer never has an alpha pixelformat. */
>>
>> Looks like above comment isn't related to graphics layer.
> Why do you think so? Because it certainly is.
Below line configures graphics layer not video layer. Why did you
comment it about video layer?
>
>
>>> + mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>>> + val, MXR_GRP_CFG_MISC_MASK);
>>> + } else {
>>> + if (cfg->index == 2) {
>>> + /*
>>> + * No blending at the moment since the NV12/NV21 pixelformats don't
>>> + * have an alpha channel. However the mixer supports a global alpha
>>> + * value for a layer. Once this functionality is exposed, we can
>>> + * support blending of the video layer through this.
>>> + */
>>> + val = 0;
>>> + mixer_reg_write(res, MXR_VIDEO_CFG, val);
>>
>> Above 'if statement' wouldn't be called because cfg->index has always a value less than 2
>> so it should be removed.
> Can you explain why you think that index is always < 2 here?
Please, see mixer_layer_blending function. In this function, a local
variable, a bottom_layer is declared and
this has false as a default. At first loop, mixer_general_layer
function won't be called
but at second loop like below,
for (i = 0; i < ctx->num_layer; ++i) {
index = ctx->layer_cfg[i].index;
/* Skip layer if it's not enabled. */
if (!(enable_state & (1 << index)))
continue;
/* Bottom layer needs special handling. */
if (bottom_layer) {
mixer_general_layer(ctx, &ctx->layer_cfg[i]); <-
and second call.
} else {
mixer_bottom_layer(ctx, &ctx->layer_cfg[i]); <-- first call.
bottom_layer = true;
}
And also, see the below declarations,
static const struct layer_cfg nonvp_default_cfg[] = {
{ .index = 0, .priority = 1 }, /* layer0 */
{ .index = 1, .priority = 2 }, /* layer1 */
};
static const struct layer_cfg vp_default_cfg[] = {
{ .index = 2, .priority = 1 }, /* video layer */
{ .index = 0, .priority = 2 }, /* layer0 */
{ .index = 1, .priority = 3 }, /* layer1 */
};
As you can see, the first position of above array has 'index = 2' so
'if (cfg->index == 2' statement of mixer_general_layer function would be true
only when mixer_general_layer function is called at first loop in
mixer_layer_blending function.
By any chance, did you consider that vp default configuration can be changed?
If so, what Exynos SoC could have such configuration?
Or how the index value could be changed in runtime?
Even you declared the above default configurations with const.
Thanks,
Inki Dae
>
>
>>> + } else {
>>> + val = MXR_GRP_CFG_COLOR_KEY_DISABLE;
>>> + mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>>> + val, MXR_GRP_CFG_MISC_MASK);
>>> + }
>>> + }
>>> +}
>>> +
>>> +static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>>> +{
>>> + unsigned int i, index;
>>> + bool bottom_layer = false;
>>> +
>>> + for (i = 0; i < ctx->num_layer; ++i) {
>>> + index = ctx->layer_cfg[i].index;
>>> +
>>> + /* Skip layer if it's not enabled. */
>>> + if (!(enable_state & (1 << index)))
>>> + continue;
>>> +
>>> + /* Bottom layer needs special handling. */
>>> + if (bottom_layer) {
>>> + mixer_general_layer(ctx, &ctx->layer_cfg[i]);
>>> + } else {
>>> + mixer_bottom_layer(ctx, &ctx->layer_cfg[i]);
>>> + bottom_layer = true;
>>> + }
>>
>> I think above codes could be more cleanned up like below if I understood correctly,
> And then we end up with the same static setup again, this is not what I
> try to achieve here. The final goal is to let userspace provide layer
> priorities, so all these calls should work with arbitrary priorities.
>
>
> With best wishes,
> Tobias
>
>
>> if (cfg->index == 2) {
>> val = 0;
>> mixer_reg_write(res, MXR_VIDEO_CFG, val);
>> } else {
>> mixer_general_layer(ctx, &ctx->layer_cfg[i]);
>> }
>>
>>
>> It'd be better to use a macro - i.e., VIDEO_LAYER - instead of 2.
>> And how about changing the function name, mixer_general_layer to mixer_grp_layer_set_blending?
>> Or how about just adding all codes of the function?
>>
>> Thanks,
>> Inki Dae
>>
>>> + }
>>> +}
>>> +
>>> static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>>> {
>>> struct mixer_resources *res = &ctx->mixer_res;
>>> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
>>> index ac60260..118872e 100644
>>> --- a/drivers/gpu/drm/exynos/regs-mixer.h
>>> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
>>> @@ -113,6 +113,7 @@
>>> #define MXR_GRP_CFG_BLEND_PRE_MUL (1 << 20)
>>> #define MXR_GRP_CFG_WIN_BLEND_EN (1 << 17)
>>> #define MXR_GRP_CFG_PIXEL_BLEND_EN (1 << 16)
>>> +#define MXR_GRP_CFG_MISC_MASK ((3 << 16) | (3 << 20))
>>> #define MXR_GRP_CFG_FORMAT_VAL(x) MXR_MASK_VAL(x, 11, 8)
>>> #define MXR_GRP_CFG_FORMAT_MASK MXR_GRP_CFG_FORMAT_VAL(~0)
>>> #define MXR_GRP_CFG_ALPHA_VAL(x) MXR_MASK_VAL(x, 7, 0)
>>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list