| Message ID | 1430405816-4183-2-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Tobias, 2015-04-30 Tobias Jakobi <tjakobi@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@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. > + > 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(). > + 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. Gustavo -- 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
Hello Gustavo! Gustavo Padovan wrote: > Hi Tobias, > > 2015-04-30 Tobias Jakobi <tjakobi@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@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. With best wishes, Tobias > > Gustavo > -- > 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 > -- 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
2015-04-30 Tobias Jakobi <liquid.acid@gmx.net>: > Hello Gustavo! > > > Gustavo Padovan wrote: > > Hi Tobias, > > > > 2015-04-30 Tobias Jakobi <tjakobi@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@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 -- 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
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; +}; + 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); + 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); + } + init_waitqueue_head(&ctx->wait_vsync_queue); atomic_set(&ctx->wait_vsync_event, 0);
First step in allowing a more generic way to setup complex blending for the different layers. Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> --- drivers/gpu/drm/exynos/exynos_mixer.c | 74 +++++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 11 deletions(-)