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

From: Sean Paul <seanpaul@chromium.org>
To: Daniel Kurtz <djkurtz@chromium.org>
Cc: "Stéphane Marchesin" <marcheu@chromium.org>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v3 19/32] drm/exynos: Use mode_set to configure fimd
Date: Wed, 4 Dec 2013 17:37:15 -0500	[thread overview]
Message-ID: <CAOw6vbKc3t9ncb0gg30MA1SzNessF=n-pDF-+ncuSGsspzPutw@mail.gmail.com> (raw)
In-Reply-To: <CAGS+omAP2LS8pS5Nybz31hO4gozqznELpmVmURgETXaVHrQ0uA@mail.gmail.com>

On Fri, Nov 15, 2013 at 8:53 AM, Daniel Kurtz <djkurtz@chromium.org> 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?
>

Seems like we'll need the patchset you just merged in the chromium
tree to fix the mismatch between userspace/kernel (to avoid the
multiple modesets on boot). I don't think it'll make any functional
difference, maybe you can post that as a followup?

>>> +
>>> +     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.
>>

That makes sense, and I originally had it this way, but changed it in
response to some review feedback. The reason it was changed was to
avoid recomputing things every flip, however that seems like a
chromium-specific hack, so I'll change it back.

Sean

>> Otherwise, the patch looks fine.
>>
>> Best regards,
>> Tomasz
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel