<div dir="ltr">Hi Sean, Tomasz,<br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 11, 2013 at 6:03 AM, Tomasz Figa <span dir="ltr"><<a href="mailto:tomasz.figa@gmail.com" target="_blank">tomasz.figa@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Sean,<br>
<div class="im"><br>
On Tuesday 29 of October 2013 12:13:05 Sean Paul wrote:<br>
> This patch uses the mode passed into mode_set to configure fimd instead<br>
> of directly using the panel from context. This will allow us to move<br>
> the exynos_drm_display implementation from fimd into the DP driver<br>
> where it belongs.<br>
<br>
</div>Remember that DP is not the only possible way to connect a display driven<br>
by FIMD. You also have the direct (RGB and i80) and DSI interfaces.<br>
<br>
Also, please see my comments inline.<br>
<div class="im"><br>
><br>
> Signed-off-by: Sean Paul <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>><br>
> ---<br>
><br>
> Changes in v2: None<br>
> Changes in v3: None<br>
><br>
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 159 ++++++++++++++++++-------------<br>
> 1 file changed, 91 insertions(+), 68 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> index d2b8ccb..f69d6e5 100644<br>
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> @@ -104,6 +104,20 @@ struct fimd_win_data {<br>
> bool resume;<br>
> };<br>
><br>
> +struct fimd_mode_data {<br>
> + unsigned vtotal;<br>
<br>
</div>For consistency with rest of the code, unsigned int would prefered.<br>
<br>
However, I'm not sure what is this struct for, since it does not store<br>
neither raw register values (1 needs to be subtracted from them) nor<br>
any values hard to compute at commit time (maybe except clkdiv, but still<br>
how often commit would be called?).<br>
<div><div class="h5"><br>
> + unsigned vdisplay;<br>
> + unsigned vsync_len;<br>
> + unsigned vbpd;<br>
> + unsigned vfpd;<br>
> + unsigned htotal;<br>
> + unsigned hdisplay;<br>
> + unsigned hsync_len;<br>
> + unsigned hbpd;<br>
> + unsigned hfpd;<br>
> + u32 clkdiv;<br>
> +};<br>
> +<br>
> struct fimd_context {<br>
> struct device *dev;<br>
> struct drm_device *drm_dev;<br>
> @@ -112,8 +126,8 @@ struct fimd_context {<br>
> struct clk *bus_clk;<br>
> struct clk *lcd_clk;<br>
> void __iomem *regs;<br>
> + struct fimd_mode_data mode;<br>
> struct fimd_win_data win_data[WINDOWS_NR];<br>
> - unsigned int clkdiv;<br>
> unsigned int default_win;<br>
> unsigned long irq_flags;<br>
> u32 vidcon0;<br>
> @@ -560,11 +574,56 @@ static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)<br>
> mutex_unlock(&ctx->lock);<br>
> }<br>
><br>
> +static u32 fimd_calc_clkdiv(struct fimd_context *ctx,<br>
> + const struct drm_display_mode *mode)<br>
> +{<br>
> + unsigned long ideal_clk = mode->htotal * mode->vtotal * mode->vrefresh;<br>
> + u32 clkdiv;<br>
> +<br>
> + /* Find the clock divider value that gets us closest to ideal_clk */<br>
> + clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(ctx->lcd_clk), ideal_clk);<br>
<br>
</div></div>This is a functional change unrelated to $subject. Before this patch,<br>
DIV_ROUND_UP() had been used.<br>
<div><div class="h5"><br>
> +<br>
> + return (clkdiv < 0x100) ? clkdiv : 0xff;<br>
> +}<br>
> +<br>
> +static bool fimd_mode_fixup(struct exynos_drm_manager *mgr,<br>
> + const struct drm_display_mode *mode,<br>
> + struct drm_display_mode *adjusted_mode)<br>
> +{<br>
> + if (adjusted_mode->vrefresh == 0)<br>
> + adjusted_mode->vrefresh = FIMD_DEFAULT_FRAMERATE;<br>
</div></div></blockquote><div><br></div><div>The actual pixel clock will be <span style="color:rgb(80,0,80)">clk_get_</span><span style="color:rgb(80,0,80)">rate(ctx->lcd_clk) / </span><span style="color:rgb(80,0,80)">clkdiv.</span></div>
<div><font color="#500050">Should we also be adjusting the "clock" field of adjusted_mode here?</font></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5">> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +static void fimd_mode_set(struct exynos_drm_manager *mgr,<br>
> + const struct drm_display_mode *in_mode)<br>
> +{<br>
> + struct fimd_context *ctx = mgr->ctx;<br>
> + struct fimd_mode_data *mode = &ctx->mode;<br>
> + int hblank, vblank;<br>
> +<br>
> + vblank = in_mode->crtc_vblank_end - in_mode->crtc_vblank_start;<br>
> + mode->vtotal = in_mode->crtc_vtotal;<br>
> + mode->vdisplay = in_mode->crtc_vdisplay;<br>
> + mode->vsync_len = in_mode->crtc_vsync_end - in_mode->crtc_vsync_start;<br>
> + mode->vbpd = (vblank - mode->vsync_len) / 2;<br>
> + mode->vfpd = vblank - mode->vsync_len - mode->vbpd;<br>
> +<br>
> + hblank = in_mode->crtc_hblank_end - in_mode->crtc_hblank_start;<br>
> + mode->htotal = in_mode->crtc_htotal;<br>
> + mode->hdisplay = in_mode->crtc_hdisplay;<br>
> + mode->hsync_len = in_mode->crtc_hsync_end - in_mode->crtc_hsync_start;<br>
> + mode->hbpd = (hblank - mode->hsync_len) / 2;<br>
> + mode->hfpd = hblank - mode->hsync_len - mode->hbpd;<br>
> +<br>
> + mode->clkdiv = fimd_calc_clkdiv(ctx, in_mode);<br>
<br>
</div></div>What about simply copying contents of in_mode to driver context<br>
and then calculating clkdiv at commit time? You could get rid<br>
of the fimd_mode_data struct and most of this function.<br>
<br>
Otherwise, the patch looks fine.<br>
<br>
Best regards,<br>
Tomasz<br>
<div class=""><div class="h5"><br>
_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</div></div></blockquote></div><br></div></div>