linux-kernel - Re: [v2] drm/sun4i: add lvds mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180420131026.vbg3qgrj26egw5s2@core>
Date:   Fri, 20 Apr 2018 15:10:26 +0200
From:   Ondřej Jirman <doudahwezomiechahtah@....cz>
To:     Giulio Benetti <giulio.benetti@...ronovasrl.com>
Cc:     Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        David Airlie <airlied@...ux.ie>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [v2] drm/sun4i: add lvds mode_valid function

Hello,

On Thu, Apr 19, 2018 at 04:02:08PM +0200, Giulio Benetti wrote:
> 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@....cz> 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@...ronovasrl.com>
> > > > ---
> > > >   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.

When I set the range of pixel clock frequencies on simple-panel connected
to this encoder, the check still fails, so there's something not working
there as expected. This check is only called once with a typical frequency.

I guess drm doesn't implement clock-frequency range on panels. But I haven't
looked.

I can set the exact frequency that the SoC can provide on the simple-panel,
but that's a bit of a hack.

regards,
  o.

> Best regards
> 
> -- 
> Giulio Benetti