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

[Giulio Benetti]

Hi everybody,

Il 19/04/2018 15:36, Chen-Yu Tsai ha scritto:
> 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/

Thanks for pointing that Thread ChenYu.
So the only chance is to trim frequency according to encoder capabilities.
I agree to block when encoder can't provide frequency specified,
otherwise you could drive you panel at the near the lowest(highest)
frequency and get out of limits for a few without knowing it.

Best regards

--
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale â 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

> 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 = {
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel