Hello Gustavo!
Gustavo Padovan wrote:
> Hi Tobias,
>
> 2015-04-30 Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>:
>
>> First step in allowing a more generic way to setup complex
>> blending for the different layers.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
