Hi Sean, Tomasz, On Mon, Nov 11, 2013 at 6:03 AM, Tomasz Figa <tomasz.figa at gmail.com> 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 <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; > 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? > > + > > + 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 > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131115/a0b458c8/attachment-0001.html>