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

[Daniel Kurtz]

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>