Re: [RFC 1/4] drm: exynos: Add infrastructure to support FIMD post processors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ajay,


On 03/19/2014 03:22 PM, Ajay Kumar wrote:
> Exynos variants support post pocessors(PP) for FIMD (MDNIE and IELCD)
> which work closely with fimd. These post processors have strict
> dependency on FIMD for poweron/poweroff. This patch adds an
> infrastructure in FIMD driver to accomodate such devices.
> 
> Added structure definition for FIMD PP and interfaces from FIMD
> driver to control FIMD PP.
> 
> Signed-off-by: Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx>
> Signed-off-by: Shirish S <s.shirish@xxxxxxxxxxx>
> Signed-off-by: Rahul Sharma <rahul.sharma@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 162 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_fimd_pp.h  |  52 ++++++++++
>  include/video/samsung_fimd.h             |   6 +-
>  3 files changed, 219 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/exynos/exynos_fimd_pp.h
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index a5511be..a584d8e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -30,6 +30,7 @@
>  #include "exynos_drm_fbdev.h"
>  #include "exynos_drm_crtc.h"
>  #include "exynos_drm_iommu.h"
> +#include "exynos_fimd_pp.h"
>  
>  /*
>   * FIMD stands for Fully Interactive Mobile Display and
> @@ -113,11 +114,14 @@ struct fimd_context {
>  	void __iomem			*regs;
>  	struct drm_display_mode		mode;
>  	struct fimd_win_data		win_data[WINDOWS_NR];
> +	struct list_head		fimd_pp_list;
>  	unsigned int			default_win;
>  	unsigned long			irq_flags;
>  	u32				vidcon0;
>  	u32				vidcon1;
>  	bool				suspended;
> +	bool				enable_pp;
> +	bool				pp_running;
>  	int				pipe;
>  	wait_queue_head_t		wait_vsync_queue;
>  	atomic_t			wait_vsync_event;
> @@ -145,6 +149,12 @@ static inline struct fimd_driver_data *drm_fimd_get_driver_data(
>  	return (struct fimd_driver_data *)of_id->data;
>  }
>  
> +void fimd_add_pp_to_list(struct fimd_context *ctx,
> +			struct exynos_fimd_pp *pp)
> +{
> +	list_add_tail(&pp->list, &ctx->fimd_pp_list);
> +}
> +
>  static int fimd_mgr_initialize(struct exynos_drm_manager *mgr,
>  			struct drm_device *drm_dev)
>  {
> @@ -214,17 +224,49 @@ static void fimd_mode_set(struct exynos_drm_manager *mgr,
>  		const struct drm_display_mode *in_mode)
>  {
>  	struct fimd_context *ctx = mgr->ctx;
> +	struct exynos_fimd_pp *pp_display;
>  
>  	drm_mode_copy(&ctx->mode, in_mode);
> +
> +	/* mode_set for the post processors*/
> +	list_for_each_entry(pp_display, &ctx->fimd_pp_list, list)
> +		if (pp_display->ops->mode_set)
> +			pp_display->ops->mode_set(pp_display->ctx, in_mode);
> +}
> +
> +static int fimd_wait_till_per_frame_off(struct fimd_context *ctx)
> +{
> +	unsigned int val = readl(ctx->regs + VIDCON0);
> +	int count = 10;
> +
> +	val &= ~(VIDCON0_ENVID_F);
> +	writel(val, ctx->regs + VIDCON0);
> +
> +	while (count) {
> +		val = readl(ctx->regs + VIDCON0);
> +		if ((val & VIDCON0_ENVID_F) == 0)
> +			break;
> +		mdelay(17);

Do we really need mdelay here? This function is not called in atomic
context, is it?


> +		count--;
> +	}
> +
> +	if (count == 0) {
> +		DRM_ERROR("FIMD per frame switch off TIMEDOUT\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
>  }

Have you tried to use interrupts/completion instead of busy-waiting
approach?

>  
>  static void fimd_commit(struct exynos_drm_manager *mgr)
>  {
>  	struct fimd_context *ctx = mgr->ctx;
>  	struct drm_display_mode *mode = &ctx->mode;
> +	struct exynos_fimd_pp *pp_display;
>  	struct fimd_driver_data *driver_data;
>  	u32 val, clkdiv, vidcon1;
>  	int hblank, vblank, vsync_len, vbpd, vfpd, hsync_len, hbpd, hfpd;
> +	int ret = 0;
>  
>  	driver_data = ctx->driver_data;
>  	if (ctx->suspended)
> @@ -234,6 +276,30 @@ static void fimd_commit(struct exynos_drm_manager *mgr)
>  	if (mode->htotal == 0 || mode->vtotal == 0)
>  		return;
>  
> +	/* The FIMD and the post processor(PP) poweroff calls below are needed
> +	 * here to make sure that fimd_commit doesn't switch on the PPs twice
> +	 * in a row. For proper working we need to keep fimd, and the PPs
> +	 * off, before we start again. This flag "pp_running" will also help
> +	 * in syncing any of the clock enable/disable calls in PP routines.
> +	 */
> +	if (ctx->enable_pp && ctx->pp_running) {
> +		if (fimd_wait_till_per_frame_off(ctx))
> +			DRM_ERROR("failed to switch off fimd per frame\n");
> +		list_for_each_entry_reverse(pp_display, &ctx->fimd_pp_list,
> +									list) {
> +			if (pp_display->ops->power_off) {
> +				ret = pp_display->ops->
> +						power_off(pp_display->ctx);
> +				if (ret) {
> +					DRM_ERROR("fimd_pp switch off fail\n");
> +					ctx->enable_pp = false;

Resetting enable_pp on power ops failures prevents disabling
DP_MDNIE_CLKCON clock on fimd power off.

> +				} else {
> +					ctx->pp_running = false;
> +				}
> +			}
> +		}
> +	}
> +

By the way, content of this big conditional maps easily to crtc and
encoder/bridge callbacks:
fimd_wait_till_per_frame_off: could be in drm_crtc_helper_funcs::prepare
power_off: could be in drm_(encoder|bridge)_helper_funcs::disable

It is just another argument for looking for common interface to
implement such blocks in more generic way, now we will have
drm_encoder, drm_bridge and exynos_fimd_pp implementing very similar
interfaces.


>  	/* setup polarity values */
>  	vidcon1 = ctx->vidcon1;
>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> @@ -286,10 +352,34 @@ static void fimd_commit(struct exynos_drm_manager *mgr)
>  	else
>  		val &= ~VIDCON0_CLKDIR;	/* 1:1 clock */
>  
> +	writel(val, ctx->regs + VIDCON0);
> +
> +	if (ctx->enable_pp && !ctx->pp_running) {
> +		/* set VCLK Free run control */
> +		val = readl(ctx->regs + VIDCON0);
> +		val |= VIDCON0_VCLKFREE;
> +		writel(val, ctx->regs + VIDCON0);
> +
> +		/* post processor setup and poweron*/
> +		list_for_each_entry(pp_display, &ctx->fimd_pp_list, list) {
> +			if (pp_display->ops->power_on) {
> +				ret = pp_display->ops->
> +						power_on(pp_display->ctx);
> +				if (ret) {
> +					DRM_ERROR("fimd_pp poweron failed\n");
> +					ctx->enable_pp = false;
> +				} else {
> +					ctx->pp_running = true;
> +				}
> +			}
> +		}
> +	}
> +

The same comment applies here.

>  	/*
>  	 * fields of register with prefix '_F' would be updated
>  	 * at vsync(same as dma start)
>  	 */
> +	val = readl(ctx->regs + VIDCON0);
>  	val |= VIDCON0_ENVID | VIDCON0_ENVID_F;
>  	writel(val, ctx->regs + VIDCON0);
>  }
> @@ -749,6 +839,9 @@ static int fimd_poweron(struct exynos_drm_manager *mgr)
>  		goto lcd_clk_err;
>  	}
>  
> +	if (ctx->enable_pp)
> +		writel(MDNIE_CLK_ENABLE, ctx->regs + DP_MDNIE_CLKCON);
> +

Name suggests that enable_pp field is for whole post-processing and
MDNIE_CLK_ENABLE is MDNIE specific, it seems to be incorrect.

>  	/* if vblank was enabled status, enable it again. */
>  	if (test_and_clear_bit(0, &ctx->irq_flags)) {
>  		ret = fimd_enable_vblank(mgr);
> @@ -776,6 +869,9 @@ bus_clk_err:
>  static int fimd_poweroff(struct exynos_drm_manager *mgr)
>  {
>  	struct fimd_context *ctx = mgr->ctx;
> +	struct exynos_fimd_pp *pp_display;
> +	int ret = 0;
> +
>  
>  	if (ctx->suspended)
>  		return 0;
> @@ -787,6 +883,27 @@ static int fimd_poweroff(struct exynos_drm_manager *mgr)
>  	 */
>  	fimd_window_suspend(mgr);
>  
> +	if (ctx->enable_pp && ctx->pp_running) {
> +		if (fimd_wait_till_per_frame_off(ctx))
> +			DRM_ERROR("failed to switch off fimd per frame\n");
> +		list_for_each_entry_reverse(pp_display, &ctx->fimd_pp_list,
> +									list) {
> +			if (pp_display->ops->power_off) {
> +				ret = pp_display->ops->
> +						power_off(pp_display->ctx);
> +				if (ret) {
> +					DRM_ERROR("fimd_pp switch off fail\n");
> +					ctx->enable_pp = false;
> +				} else {
> +					ctx->pp_running = false;
> +				}
> +			}
> +		}
> +	}

The same loop again, separate function could be used instead.

> +
> +	if (ctx->enable_pp)
> +		writel(0, ctx->regs + DP_MDNIE_CLKCON);
> +

Again, mixing different level of abstraction.


>  	clk_disable_unprepare(ctx->lcd_clk);
>  	clk_disable_unprepare(ctx->bus_clk);
>  
> @@ -815,6 +932,47 @@ static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
>  	}
>  }
>  
> +static int fimd_set_property(void *in_ctx, struct drm_property *property,
> +								uint64_t val)
> +{
> +	struct fimd_context *ctx = in_ctx;
> +	struct exynos_fimd_pp *pp_display;
> +	int ret;
> +
> +	list_for_each_entry(pp_display, &ctx->fimd_pp_list, list) {
> +		if (pp_display->ops->set_property) {
> +			ret = pp_display->ops->set_property(ctx->drm_dev,
> +					pp_display->ctx, property, val);
> +			if (ret) {
> +				DRM_ERROR("pp set_property failed.\n");
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int fimd_set_gamma(void *in_ctx, u16 *r, u16 *g, u16 *b,
> +					uint32_t start, uint32_t size)
> +{
> +	struct fimd_context *ctx = in_ctx;
> +	struct exynos_fimd_pp *pp_display;
> +	int ret;
> +
> +	list_for_each_entry(pp_display, &ctx->fimd_pp_list, list) {
> +		if (pp_display->ops->set_property)

s/property/gamma/;

> +			ret = pp_display->ops->set_gamma(pp_display->ctx,
> +							r, g, b, start, size);
> +			if (ret) {
> +				DRM_ERROR("pp set_gamma failed.\n");
> +				return ret;
> +			}
> +	}
> +
> +	return 0;
> +}
> +
>  static struct exynos_drm_manager_ops fimd_manager_ops = {
>  	.dpms = fimd_dpms,
>  	.mode_fixup = fimd_mode_fixup,
> @@ -826,6 +984,8 @@ static struct exynos_drm_manager_ops fimd_manager_ops = {
>  	.win_mode_set = fimd_win_mode_set,
>  	.win_commit = fimd_win_commit,
>  	.win_disable = fimd_win_disable,
> +	.set_property = fimd_set_property,
> +	.set_gamma = fimd_set_gamma,
>  };

.set_property and .set_gamma fields are not present in
exynos_drm_manager_ops in exynos-drm-next-todo branch, I guess some
patches are missing.

>  
>  static struct exynos_drm_manager fimd_manager = {
> @@ -919,6 +1079,8 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
>  	init_waitqueue_head(&ctx->wait_vsync_queue);
>  	atomic_set(&ctx->wait_vsync_event, 0);
>  
> +	INIT_LIST_HEAD(&ctx->fimd_pp_list);
> +
>  	platform_set_drvdata(pdev, &fimd_manager);
>  
>  	fimd_manager.ctx = ctx;
> diff --git a/drivers/gpu/drm/exynos/exynos_fimd_pp.h b/drivers/gpu/drm/exynos/exynos_fimd_pp.h
> new file mode 100644
> index 0000000..528d3cb
> --- /dev/null
> +++ b/drivers/gpu/drm/exynos/exynos_fimd_pp.h
> @@ -0,0 +1,52 @@
> +/* drivers/gpu/drm/exynos/exynos_fimd_pp.h
> + *
> + * Copyright (C) 2014 Samsung Electronics Co.Ltd
> + *
> + * Header file for FIMD post processors.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#ifndef __EXYNOS_FIMD_H__
> +#define __EXYNOS_FIMD_H__
> +
> +#include <drm/exynos_drm.h>
> +
> +/**
> + * Post Processor interfaces structure for DRM based FIMD.
> + *
> + * @power_on: setup the post processor.
> + * @power_off: power off the post processor.
> + * @mode_set: set video timings if needed by the post processor.
> + * @set_property: use crtc properties to set the post processor.
> + * @set_gamma: set gamma properties to post processor.
> + *
> + */
> +struct exynos_fimd_pp_ops {
> +	int (*power_on)(void *pp_ctx);
> +	int (*power_off)(void *pp_ctx);
> +	void (*mode_set)(void *pp_ctx, const struct drm_display_mode *in_mode);
> +	int (*set_property)(struct drm_device *drm_dev, void *pp_ctx,
> +				struct drm_property *property, uint64_t val);

I wonder if it would not be better to use drm_crtc instead of drm_device
here, it seems to be more consistent with drm_crtc_funcs:set_property.


Regards
Andrzej

> +	int (*set_gamma)(void *pp_ctx, u16 *r,
> +			u16 *g, u16 *b, uint32_t start, uint32_t size);
> +};
> +
> +/*
> + * Exynos FIMD post processor structure
> + * @list: the list entry
> + * @ops: pointer to callbacks for post processor specific functionality
> + * @ctx: A pointer to the post processor's implementation specific context
> + *
> + */
> +struct exynos_fimd_pp {
> +	struct list_head list;
> +	struct exynos_fimd_pp_ops *ops;
> +	void *ctx;
> +};
> +
> +#endif
> diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
> index b039320..3ff7cad 100644
> --- a/include/video/samsung_fimd.h
> +++ b/include/video/samsung_fimd.h
> @@ -60,7 +60,7 @@
>  #define VIDCON0_CLKVAL_F_SHIFT			6
>  #define VIDCON0_CLKVAL_F_LIMIT			0xff
>  #define VIDCON0_CLKVAL_F(_x)			((_x) << 6)
> -#define VIDCON0_VLCKFREE			(1 << 5)
> +#define VIDCON0_VCLKFREE			(1 << 5)
>  #define VIDCON0_CLKDIR				(1 << 4)
>  
>  #define VIDCON0_CLKSEL_MASK			(0x3 << 2)
> @@ -435,6 +435,10 @@
>  #define BLENDCON_NEW_8BIT_ALPHA_VALUE		(1 << 0)
>  #define BLENDCON_NEW_4BIT_ALPHA_VALUE		(0 << 0)
>  
> +/* DP_MDNIE_CLKCON */
> +#define DP_MDNIE_CLKCON				0x27c
> +#define MDNIE_CLK_ENABLE				0x3
> +
>  /* Notes on per-window bpp settings
>   *
>   * Value	Win0	 Win1	  Win2	   Win3	    Win 4
> 

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux