From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v3 19/32] drm/exynos: Use mode_set to configure fimd Date: Thu, 28 Nov 2013 23:57:38 +0100 Message-ID: <2889487.X5QZjpcy4y@flatron> References: <1383063198-10526-1-git-send-email-seanpaul@chromium.org> <2503660.zt59Ypmxx4@flatron> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f171.google.com (mail-ea0-f171.google.com [209.85.215.171]) by gabe.freedesktop.org (Postfix) with ESMTP id E9140FA38A for ; Thu, 28 Nov 2013 14:57:36 -0800 (PST) Received: by mail-ea0-f171.google.com with SMTP id h10so6313265eak.30 for ; Thu, 28 Nov 2013 14:57:36 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Daniel Kurtz Cc: =?ISO-8859-1?Q?St=E9phane?= Marchesin , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Friday 15 of November 2013 21:53:16 Daniel Kurtz wrote: > Hi Sean, Tomasz, > > On Mon, Nov 11, 2013 at 6:03 AM, Tomasz Figa wrote: > > 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 > >> --- > >> > >> 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; > > The actual pixel clock will be clk_get_rate(ctx->lcd_clk) / clkdiv. > Should we also be adjusting the "clock" field of adjusted_mode here? Not sure how the pixel clock in adjusted_mode is used elsewhere, but at least for the sake of consistency, it might be good idea to adjust it indeed. Best regards, Tomasz