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

[Tomasz Figa <tomasz.figa@gmail.com>]

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 <tomasz.figa@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@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?

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