Re: [v2] drm/sun4i: add lvds mode_valid function

[Chen-Yu Tsai]

On Thu, Apr 19, 2018 at 9:34 PM, OndÅej Jirman
<doudahwezomiechahtah@xxxxxx> wrote:
> Hello Giulio,
>
> this patch breaks LVDS output on A83T. Without it, modesetting works,
> with it there's no output.
>
> Some more info below...
>
> On Tue, Mar 13, 2018 at 12:20:19PM +0100, Giulio Benetti wrote:
>> mode_valid function is missing for lvds.
>>
>> Add it making it pointed by encoder helper functions.
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti@xxxxxxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/sun4i/sun4i_lvds.c | 55 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
>> index be3f14d..bffff4c 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
>> @@ -94,9 +94,64 @@ static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
>>       }
>>  }
>>
>> +static enum drm_mode_status sun4i_lvds_encoder_mode_valid(struct drm_encoder *crtc,
>> +                                                       const struct drm_display_mode *mode)
>> +{
>> +     struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(crtc);
>> +     struct sun4i_tcon *tcon = lvds->tcon;
>> +     u32 hsync = mode->hsync_end - mode->hsync_start;
>> +     u32 vsync = mode->vsync_end - mode->vsync_start;
>> +     unsigned long rate = mode->clock * 1000;
>> +     long rounded_rate;
>> +
>> +     DRM_DEBUG_DRIVER("Validating modes...\n");
>> +
>> +     if (hsync < 1)
>> +             return MODE_HSYNC_NARROW;
>> +
>> +     if (hsync > 0x3ff)
>> +             return MODE_HSYNC_WIDE;
>> +
>> +     if ((mode->hdisplay < 1) || (mode->htotal < 1))
>> +             return MODE_H_ILLEGAL;
>> +
>> +     if ((mode->hdisplay > 0x7ff) || (mode->htotal > 0xfff))
>> +             return MODE_BAD_HVALUE;
>> +
>> +     DRM_DEBUG_DRIVER("Horizontal parameters OK\n");
>> +
>> +     if (vsync < 1)
>> +             return MODE_VSYNC_NARROW;
>> +
>> +     if (vsync > 0x3ff)
>> +             return MODE_VSYNC_WIDE;
>> +
>> +     if ((mode->vdisplay < 1) || (mode->vtotal < 1))
>> +             return MODE_V_ILLEGAL;
>> +
>> +     if ((mode->vdisplay > 0x7ff) || (mode->vtotal > 0xfff))
>> +             return MODE_BAD_VVALUE;
>> +
>> +     DRM_DEBUG_DRIVER("Vertical parameters OK\n");
>> +
>> +     tcon->dclk_min_div = 7;
>> +     tcon->dclk_max_div = 7;
>
> Why would validation function change any state anywhere?
>
>> +     rounded_rate = clk_round_rate(tcon->dclk, rate);
>
> The issue is here, on A83T TBS A711 tablet, I get...
>
> sun4i-tcon 1c0c000.lcd-controller: XXX: hsync=20 hdisplay=1024 htotal=1384
>   vsync=5 vdisplay=600 vtotal=640 rate=52000000 rounded_rate=51857142
>
>> +     if (rounded_rate < rate)
>> +             return MODE_CLOCK_LOW;
>> +
>> +     if (rounded_rate > rate)
>> +             return MODE_CLOCK_HIGH;
>
> ... while the previous conditions require an exact match for some reason.
>
> But HW works fine without an exact rate match. Why is exact match required here?

This thread might provide some more info, though we could never get an
agreement.

https://patchwork.kernel.org/patch/9446385/

ChenYu

>
> thank you,
>   Ondrej
>
>> +     DRM_DEBUG_DRIVER("Clock rate OK\n");
>> +
>> +     return MODE_OK;
>> +}
>> +
>>  static const struct drm_encoder_helper_funcs sun4i_lvds_enc_helper_funcs = {
>>       .disable        = sun4i_lvds_encoder_disable,
>>       .enable         = sun4i_lvds_encoder_enable,
>> +     .mode_valid     = sun4i_lvds_encoder_mode_valid,
>>  };
>>
>>  static const struct drm_encoder_funcs sun4i_lvds_enc_funcs = {