* Re: [v2] drm/sun4i: add lvds mode_valid function
@ 2018-04-19 13:36 ` Chen-Yu Tsai
0 siblings, 0 replies; 17+ messages in thread
From: Chen-Yu Tsai @ 2018-04-19 13:36 UTC (permalink / raw)
To: Giulio Benetti, Maxime Ripard, David Airlie, linux-kernel,
dri-devel, Chen-Yu Tsai, linux-arm-kernel
On Thu, Apr 19, 2018 at 9:34 PM, Ondřej Jirman
<doudahwezomiechahtah@xff.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@micronovasrl.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/
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* [v2] drm/sun4i: add lvds mode_valid function
@ 2018-04-19 13:36 ` Chen-Yu Tsai
0 siblings, 0 replies; 17+ messages in thread
From: Chen-Yu Tsai @ 2018-04-19 13:36 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Apr 19, 2018 at 9:34 PM, Ond?ej Jirman
<doudahwezomiechahtah@xff.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@micronovasrl.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/
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 = {
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [v2] drm/sun4i: add lvds mode_valid function
2018-04-19 13:36 ` Chen-Yu Tsai
@ 2018-04-19 14:02 ` Giulio Benetti
-1 siblings, 0 replies; 17+ messages in thread
From: Giulio Benetti @ 2018-04-19 14:02 UTC (permalink / raw)
To: Chen-Yu Tsai, Maxime Ripard, David Airlie, linux-kernel,
dri-devel, linux-arm-kernel
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@xff.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@micronovasrl.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.
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 17+ messages in thread* [v2] drm/sun4i: add lvds mode_valid function
@ 2018-04-19 14:02 ` Giulio Benetti
0 siblings, 0 replies; 17+ messages in thread
From: Giulio Benetti @ 2018-04-19 14:02 UTC (permalink / raw)
To: linux-arm-kernel
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@xff.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@micronovasrl.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.
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [v2] drm/sun4i: add lvds mode_valid function
2018-04-19 14:02 ` Giulio Benetti
@ 2018-04-20 13:10 ` Ondřej Jirman
-1 siblings, 0 replies; 17+ messages in thread
From: Ondřej Jirman @ 2018-04-20 13:10 UTC (permalink / raw)
To: Giulio Benetti
Cc: Chen-Yu Tsai, Maxime Ripard, David Airlie, linux-kernel,
dri-devel, linux-arm-kernel
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@xff.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@micronovasrl.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
^ permalink raw reply [flat|nested] 17+ messages in thread* [v2] drm/sun4i: add lvds mode_valid function
@ 2018-04-20 13:10 ` Ondřej Jirman
0 siblings, 0 replies; 17+ messages in thread
From: Ondřej Jirman @ 2018-04-20 13:10 UTC (permalink / raw)
To: linux-arm-kernel
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@xff.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@micronovasrl.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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [v2] drm/sun4i: add lvds mode_valid function
2018-04-20 13:10 ` Ondřej Jirman
@ 2018-04-20 20:39 ` Maxime Ripard
-1 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-04-20 20:39 UTC (permalink / raw)
To: Giulio Benetti, Chen-Yu Tsai, David Airlie, linux-kernel,
dri-devel, linux-arm-kernel
On Fri, Apr 20, 2018 at 03:10:26PM +0200, Ondřej Jirman wrote:
> 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@xff.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@micronovasrl.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.
It does, but only omapdss is able to use it iirc. We could do it too,
but that would be way out of scope for a fix.
> I can set the exact frequency that the SoC can provide on the simple-panel,
> but that's a bit of a hack.
Yes. And if the only device using this thus far is broken, that's not
really great either.
Can you send a revert?
Thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 17+ messages in thread* [v2] drm/sun4i: add lvds mode_valid function
@ 2018-04-20 20:39 ` Maxime Ripard
0 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-04-20 20:39 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 20, 2018 at 03:10:26PM +0200, Ond?ej Jirman wrote:
> 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@xff.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@micronovasrl.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.
It does, but only omapdss is able to use it iirc. We could do it too,
but that would be way out of scope for a fix.
> I can set the exact frequency that the SoC can provide on the simple-panel,
> but that's a bit of a hack.
Yes. And if the only device using this thus far is broken, that's not
really great either.
Can you send a revert?
Thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 17+ messages in thread