Hi Sean, Tomasz,
On Mon, Nov 11, 2013 at 6:03 AM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
Hi Sean,
Remember that DP is not the only possible way to connect a display driven
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.
by FIMD. You also have the direct (RGB and i80) and DSI interfaces.
Also, please see my comments inline.
For consistency with rest of the code, unsigned int would prefered.
>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
>
> 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;
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?).
This is a functional change unrelated to $subject. Before this patch,
> + 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);
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?
What about simply copying contents of in_mode to driver context> +
> + 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);
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
