[PATCH v3 19/32] drm/exynos: Use mode_set to configure fimd

[Tomasz Figa]

Hi Sean,

On Tuesday 29 of October 2013 12:13:05 Sean Paul wrote:
> This patch uses the mode passed into mode_set to configure fimd instead
> of directly using the panel from context. This will allow us to move
> the exynos_drm_display implementation from fimd into the DP driver
> where it belongs.

Remember that DP is not the only possible way to connect a display driven
by FIMD. You also have the direct (RGB and i80) and DSI interfaces.

Also, please see my comments inline.

> 
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> 
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 159 ++++++++++++++++++-------------
>  1 file changed, 91 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index d2b8ccb..f69d6e5 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -104,6 +104,20 @@ struct fimd_win_data {
>  	bool			resume;
>  };
>  
> +struct fimd_mode_data {
> +	unsigned		vtotal;

For consistency with rest of the code, unsigned int would prefered.

However, I'm not sure what is this struct for, since it does not store
neither raw register values (1 needs to be subtracted from them) nor
any values hard to compute at commit time (maybe except clkdiv, but still
how often commit would be called?).

> +	unsigned		vdisplay;
> +	unsigned		vsync_len;
> +	unsigned		vbpd;
> +	unsigned		vfpd;
> +	unsigned		htotal;
> +	unsigned		hdisplay;
> +	unsigned		hsync_len;
> +	unsigned		hbpd;
> +	unsigned		hfpd;
> +	u32			clkdiv;
> +};
> +
>  struct fimd_context {
>  	struct device			*dev;
>  	struct drm_device		*drm_dev;
> @@ -112,8 +126,8 @@ struct fimd_context {
>  	struct clk			*bus_clk;
>  	struct clk			*lcd_clk;
>  	void __iomem			*regs;
> +	struct fimd_mode_data		mode;
>  	struct fimd_win_data		win_data[WINDOWS_NR];
> -	unsigned int			clkdiv;
>  	unsigned int			default_win;
>  	unsigned long			irq_flags;
>  	u32				vidcon0;
> @@ -560,11 +574,56 @@ static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
>  	mutex_unlock(&ctx->lock);
>  }
>  
> +static u32 fimd_calc_clkdiv(struct fimd_context *ctx,
> +		const struct drm_display_mode *mode)
> +{
> +	unsigned long ideal_clk = mode->htotal * mode->vtotal * mode->vrefresh;
> +	u32 clkdiv;
> +
> +	/* Find the clock divider value that gets us closest to ideal_clk */
> +	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(ctx->lcd_clk), ideal_clk);

This is a functional change unrelated to $subject. Before this patch,
DIV_ROUND_UP() had been used.

> +
> +	return (clkdiv < 0x100) ? clkdiv : 0xff;
> +}
> +
> +static bool fimd_mode_fixup(struct exynos_drm_manager *mgr,
> +		const struct drm_display_mode *mode,
> +		struct drm_display_mode *adjusted_mode)
> +{
> +	if (adjusted_mode->vrefresh == 0)
> +		adjusted_mode->vrefresh = FIMD_DEFAULT_FRAMERATE;
> +
> +	return true;
> +}
> +
> +static void fimd_mode_set(struct exynos_drm_manager *mgr,
> +		const struct drm_display_mode *in_mode)
> +{
> +	struct fimd_context *ctx = mgr->ctx;
> +	struct fimd_mode_data *mode = &ctx->mode;
> +	int hblank, vblank;
> +
> +	vblank = in_mode->crtc_vblank_end - in_mode->crtc_vblank_start;
> +	mode->vtotal = in_mode->crtc_vtotal;
> +	mode->vdisplay = in_mode->crtc_vdisplay;
> +	mode->vsync_len = in_mode->crtc_vsync_end - in_mode->crtc_vsync_start;
> +	mode->vbpd = (vblank - mode->vsync_len) / 2;
> +	mode->vfpd = vblank - mode->vsync_len - mode->vbpd;
> +
> +	hblank = in_mode->crtc_hblank_end - in_mode->crtc_hblank_start;
> +	mode->htotal = in_mode->crtc_htotal;
> +	mode->hdisplay = in_mode->crtc_hdisplay;
> +	mode->hsync_len = in_mode->crtc_hsync_end - in_mode->crtc_hsync_start;
> +	mode->hbpd = (hblank - mode->hsync_len) / 2;
> +	mode->hfpd = hblank - mode->hsync_len - mode->hbpd;
> +
> +	mode->clkdiv = fimd_calc_clkdiv(ctx, in_mode);

What about simply copying contents of in_mode to driver context
and then calculating clkdiv at commit time? You could get rid
of the fimd_mode_data struct and most of this function.

Otherwise, the patch looks fine.

Best regards,
Tomasz