2015-04-30 Tobias Jakobi <liquid.acid at gmx.net>:
> Hello Gustavo!
>
>
> Gustavo Padovan wrote:
> > Hi Tobias,
> >
> > 2015-04-30 Tobias Jakobi <tjakobi at math.uni-bielefeld.de>:
> >
> >> First step in allowing a more generic way to setup complex
> >> blending for the different layers.
> >>
> >> Signed-off-by: Tobias Jakobi <tjakobi at math.uni-bielefeld.de>
> >> ---
> >> drivers/gpu/drm/exynos/exynos_mixer.c | 74
> >> +++++++++++++++++++++++++++++------
> >> 1 file changed, 63 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
> >> b/drivers/gpu/drm/exynos/exynos_mixer.c
> >> index 4155f43..a06b8e2 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> >> @@ -63,6 +63,12 @@ struct mixer_resources {
> >> struct clk *mout_mixer;
> >> };
> >>
> >> +struct layer_config {
> >> + unsigned int index;
> >> + unsigned int priority;
> >> + u32 cfg;
> >> +};
> >
> > I don't see why you are creating this struct, index and priority are
> > never used in this patch series.
> Good catch about 'priority'. But 'index' is used, see the second patch.
>
>
>
> >
> >> +
> >> enum mixer_version_id {
> >> MXR_VER_0_0_0_16,
> >> MXR_VER_16_0_33_0,
> >> @@ -75,6 +81,8 @@ struct mixer_context {
> >> struct drm_device *drm_dev;
> >> struct exynos_drm_crtc *crtc;
> >> struct exynos_drm_plane planes[MIXER_WIN_NR];
> >> + const struct layer_config *layer_config;
> >> + unsigned int num_layer;
> >> int pipe;
> >> bool interlace;
> >> bool powered;
> >> @@ -95,6 +103,40 @@ struct mixer_drv_data {
> >> bool has_sclk;
> >> };
> >>
> >> +/*
> >> + * The default layer priorities. A higher priority means that
> >> + * the layer is at the top of layer stack.
> >> + * The current configuration assumes the following usage scenario:
> >> + * layer1: OSD [top]
> >> + * layer0: main framebuffer
> >> + * video layer: video overlay [bottom]
> >> + * Note that the video layer is only usable when the
> >> + * video processor is available.
> >> + */
> >> +
> >> +static const struct layer_config default_layer_config[] = {
> >> + {
> >> + .index = 0, .priority = 1, /* layer0 */
> >> + .cfg = MXR_LAYER_CFG_GRP0_VAL(1)
> >> + }, {
> >> + .index = 1, .priority = 2, /* layer1 */
> >> + .cfg = MXR_LAYER_CFG_GRP1_VAL(2)
> >> + }
> >> +};
> >> +
> >> +static const struct layer_config vp_layer_config[] = {
> >> + {
> >> + .index = 2, .priority = 1, /* video layer */
> >> + .cfg = MXR_LAYER_CFG_VP_VAL(1)
> >> + }, {
> >> + .index = 0, .priority = 2, /* layer0 */
> >> + .cfg = MXR_LAYER_CFG_GRP0_VAL(2)
> >> + }, {
> >> + .index = 1, .priority = 3, /* layer1 */
> >> + .cfg = MXR_LAYER_CFG_GRP1_VAL(3)
> >> + }
> >> +};
> >> +
> >> static const u8 filter_y_horiz_tap8[] = {
> >> 0, -1, -1, -1, -1, -1, -1, -1,
> >> -1, -1, -1, -1, -1, 0, 0, 0,
> >> @@ -253,6 +295,17 @@ static void vp_default_filter(struct mixer_resources
> >> *res)
> >> filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
> >> }
> >>
> >> +static void mixer_layer_priority(struct mixer_context *ctx)
> >> +{
> >> + u32 val = 0;
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < ctx->num_layer; ++i)
> >> + val |= ctx->layer_config[i].cfg;
> >> +
> >> + mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
> >> +}
> >> +
> >> static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
> >> {
> >> struct mixer_resources *res = &ctx->mixer_res;
> >> @@ -655,17 +708,7 @@ static void mixer_win_reset(struct mixer_context *ctx)
> >> mixer_reg_writemask(res, MXR_STATUS, MXR_STATUS_16_BURST,
> >> MXR_STATUS_BURST_MASK);
> >>
> >> - /* setting default layer priority: layer1 > layer0 > video
> >> - * because typical usage scenario would be
> >> - * layer1 - OSD
> >> - * layer0 - framebuffer
> >> - * video - video overlay
> >> - */
> >> - val = MXR_LAYER_CFG_GRP1_VAL(3);
> >> - val |= MXR_LAYER_CFG_GRP0_VAL(2);
> >> - if (ctx->vp_enabled)
> >> - val |= MXR_LAYER_CFG_VP_VAL(1);
> >> - mixer_reg_write(res, MXR_LAYER_CFG, val);
> >
> > I would move this exaclty piece of code into mixer_layer_priority().
> Then we end up with the same static/hardcoded setup as before. That's
> something I want to move away from. The entire information about layer
> ordering should be stored in 'layer_config'.
>
>
>
> >> + mixer_layer_priority(ctx);
> >>
> >> /* setting background color */
> >> mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
> >> @@ -1274,6 +1317,15 @@ static int mixer_probe(struct platform_device *pdev)
> >> ctx->vp_enabled = drv->is_vp_enabled;
> >> ctx->has_sclk = drv->has_sclk;
> >> ctx->mxr_ver = drv->version;
> >> +
> >> + if (drv->is_vp_enabled) {
> >> + ctx->layer_config = vp_layer_config;
> >> + ctx->num_layer = ARRAY_SIZE(vp_layer_config);
> >> + } else {
> >> + ctx->layer_config = default_layer_config;
> >> + ctx->num_layer = ARRAY_SIZE(default_layer_config);
> >> + }
> >
> > Then this piece of code is useless.
> No, since the second patch depends on it.
Right, I did a second look on patch 2 and realized that index is still
used and this code is actually needed here.
Gustavo
