[PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[Alexey Brodkin]

Since we cannot always generate exactly requested pixel clock
there's not much sense in checking requested_clock == clk_round_rate().
In that case for quite some modes we'll be getting -EINVAL and no video
output at all.

But given there's some tolerance to real pixel clock in TVs/monitors
we may still give it a try with the clock as close to requested one as
PLL on the board may generate. So we just do a fixup to what current
board may provide.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Jose Abreu <joabreu@synopsys.com>
---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a95916f1f..3f2823c1efc3 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -129,18 +129,16 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
 			      ~ARCPGU_CTRL_ENABLE_MASK);
 }
 
-static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
-				     struct drm_crtc_state *state)
+static bool arc_pgu_crtc_mode_fixup(struct drm_crtc *crtc,
+				    const struct drm_display_mode *mode,
+				    struct drm_display_mode *adjusted_mode)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-	struct drm_display_mode *mode = &state->adjusted_mode;
-	long rate, clk_rate = mode->clock * 1000;
 
-	rate = clk_round_rate(arcpgu->clk, clk_rate);
-	if (rate != clk_rate)
-		return -EINVAL;
+	adjusted_mode->clock =
+		clk_round_rate(arcpgu->clk, mode->clock * 1000) / 1000;
 
-	return 0;
+	return true;
 }
 
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
@@ -165,8 +163,8 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
 	.disable	= arc_pgu_crtc_disable,
 	.prepare	= arc_pgu_crtc_disable,
 	.commit		= arc_pgu_crtc_enable,
-	.atomic_check	= arc_pgu_crtc_atomic_check,
 	.atomic_begin	= arc_pgu_crtc_atomic_begin,
+	.mode_fixup	= arc_pgu_crtc_mode_fixup,
 };
 
 static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
-- 
2.7.4

Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[Daniel Vetter]

On Thu, Mar 02, 2017 at 08:27:54PM +0300, Alexey Brodkin wrote:
> Since we cannot always generate exactly requested pixel clock
> there's not much sense in checking requested_clock == clk_round_rate().
> In that case for quite some modes we'll be getting -EINVAL and no video
> output at all.
> 
> But given there's some tolerance to real pixel clock in TVs/monitors
> we may still give it a try with the clock as close to requested one as
> PLL on the board may generate. So we just do a fixup to what current
> board may provide.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Jose Abreu <joabreu@synopsys.com>
> ---
>  drivers/gpu/drm/arc/arcpgu_crtc.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index ad9a95916f1f..3f2823c1efc3 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -129,18 +129,16 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
>  			      ~ARCPGU_CTRL_ENABLE_MASK);
>  }
>  
> -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> -				     struct drm_crtc_state *state)
> +static bool arc_pgu_crtc_mode_fixup(struct drm_crtc *crtc,
> +				    const struct drm_display_mode *mode,
> +				    struct drm_display_mode *adjusted_mode)

This isn't required at all, see drm_crtc_state.adjusted_mode. Just update
that and you're good - .mode_fixup is the backwards compatibility function
for old kms drivers, atomic_check is strictly more powerful.

Please also make sure the documentation properly explains this, and if
not, please submit a patch to improve it.
-Daniel

>  {
>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> -	struct drm_display_mode *mode = &state->adjusted_mode;
> -	long rate, clk_rate = mode->clock * 1000;
>  
> -	rate = clk_round_rate(arcpgu->clk, clk_rate);
> -	if (rate != clk_rate)
> -		return -EINVAL;
> +	adjusted_mode->clock =
> +		clk_round_rate(arcpgu->clk, mode->clock * 1000) / 1000;
>  
> -	return 0;
> +	return true;
>  }
>  
>  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -165,8 +163,8 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
>  	.disable	= arc_pgu_crtc_disable,
>  	.prepare	= arc_pgu_crtc_disable,
>  	.commit		= arc_pgu_crtc_enable,
> -	.atomic_check	= arc_pgu_crtc_atomic_check,
>  	.atomic_begin	= arc_pgu_crtc_atomic_begin,
> +	.mode_fixup	= arc_pgu_crtc_mode_fixup,
>  };
>  
>  static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[Alexey Brodkin]

Hi Daniel,

On Thu, 2017-03-02 at 20:54 +0100, Daniel Vetter wrote:
> On Thu, Mar 02, 2017 at 08:27:54PM +0300, Alexey Brodkin wrote:
> > 
> > Since we cannot always generate exactly requested pixel clock
> > there's not much sense in checking requested_clock == clk_round_rate().
> > In that case for quite some modes we'll be getting -EINVAL and no video
> > output at all.
> > 
> > But given there's some tolerance to real pixel clock in TVs/monitors
> > we may still give it a try with the clock as close to requested one as
> > PLL on the board may generate. So we just do a fixup to what current
> > board may provide.
> > 
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Jose Abreu <joabreu@synopsys.com>
> > ---
> >  drivers/gpu/drm/arc/arcpgu_crtc.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > index ad9a95916f1f..3f2823c1efc3 100644
> > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > @@ -129,18 +129,16 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
> >  			      ~ARCPGU_CTRL_ENABLE_MASK);
> >  }
> >  
> > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> > -				     struct drm_crtc_state *state)
> > +static bool arc_pgu_crtc_mode_fixup(struct drm_crtc *crtc,
> > +				    const struct drm_display_mode *mode,
> > +				    struct drm_display_mode *adjusted_mode)
> 
> This isn't required at all, see drm_crtc_state.adjusted_mode. Just update
> that and you're good - .mode_fixup is the backwards compatibility function
> for old kms drivers, atomic_check is strictly more powerful.

So if I understood you correct here what I really need is just to get rid of existing check,
right? I.e. the following is to be in v2 respin:
------------------------------->8-------------------------------
diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a95916f1f..86f1555914e8 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -129,20 +129,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
                              ~ARCPGU_CTRL_ENABLE_MASK);
 }
 
-static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
-                                    struct drm_crtc_state *state)
-{
-       struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-       struct drm_display_mode *mode = &state->adjusted_mode;
-       long rate, clk_rate = mode->clock * 1000;
-
-       rate = clk_round_rate(arcpgu->clk, clk_rate);
-       if (rate != clk_rate)
-               return -EINVAL;
-
-       return 0;
-}
-
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
                                      struct drm_crtc_state *state)
 {
@@ -165,7 +151,6 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
        .disable        = arc_pgu_crtc_disable,
        .prepare        = arc_pgu_crtc_disable,
        .commit         = arc_pgu_crtc_enable,
-       .atomic_check   = arc_pgu_crtc_atomic_check,
        .atomic_begin   = arc_pgu_crtc_atomic_begin,
 };
------------------------------->8-------------------------------

> Please also make sure the documentation properly explains this, and if
> not, please submit a patch to improve it.

You mean explains what? That .mode_fixup is not meant to be used in
new code?

-Alexey

Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[Jose Abreu]

Hi Alexey,


On 03-03-2017 13:27, Alexey Brodkin wrote:
>
> So if I understood you correct here what I really need is just to get rid of existing check,
> right? I.e. the following is to be in v2 respin:
> ------------------------------->8-------------------------------
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index ad9a95916f1f..86f1555914e8 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -129,20 +129,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
>                               ~ARCPGU_CTRL_ENABLE_MASK);
>  }
>  
> -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> -                                    struct drm_crtc_state *state)
> -{
> -       struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> -       struct drm_display_mode *mode = &state->adjusted_mode;
> -       long rate, clk_rate = mode->clock * 1000;
> -
> -       rate = clk_round_rate(arcpgu->clk, clk_rate);
> -       if (rate != clk_rate)
> -               return -EINVAL;
> -
> -       return 0;
> -}
> -
>  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>                                       struct drm_crtc_state *state)
>  {
> @@ -165,7 +151,6 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
>         .disable        = arc_pgu_crtc_disable,
>         .prepare        = arc_pgu_crtc_disable,
>         .commit         = arc_pgu_crtc_enable,
> -       .atomic_check   = arc_pgu_crtc_atomic_check,
>         .atomic_begin   = arc_pgu_crtc_atomic_begin,
>  };
> ------------------------------->8-------------------------------

I don't think you can remove the check entirely as this will make
any mode be accepted, right?

Best regards,
Jose Miguel Abreu

Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[Alexey Brodkin]

Hi Jose,

On Fri, 2017-03-03 at 18:05 +0000, Jose Abreu wrote:
> Hi Alexey,
> 
> 
> On 03-03-2017 13:27, Alexey Brodkin wrote:
> > 
> > 
> > So if I understood you correct here what I really need is just to get rid of existing check,
> > right? I.e. the following is to be in v2 respin:
> > ------------------------------->8-------------------------------
> > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > index ad9a95916f1f..86f1555914e8 100644
> > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > @@ -129,20 +129,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
> >                               ~ARCPGU_CTRL_ENABLE_MASK);
> >  }
> >  
> > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> > -                                    struct drm_crtc_state *state)
> > -{
> > -       struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> > -       struct drm_display_mode *mode = &state->adjusted_mode;
> > -       long rate, clk_rate = mode->clock * 1000;
> > -
> > -       rate = clk_round_rate(arcpgu->clk, clk_rate);
> > -       if (rate != clk_rate)
> > -               return -EINVAL;
> > -
> > -       return 0;
> > -}
> > -
> >  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
> >                                       struct drm_crtc_state *state)
> >  {
> > @@ -165,7 +151,6 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> >         .disable        = arc_pgu_crtc_disable,
> >         .prepare        = arc_pgu_crtc_disable,
> >         .commit         = arc_pgu_crtc_enable,
> > -       .atomic_check   = arc_pgu_crtc_atomic_check,
> >         .atomic_begin   = arc_pgu_crtc_atomic_begin,
> >  };
> > ------------------------------->8-------------------------------
> 
> I don't think you can remove the check entirely as this will make
> any mode be accepted, right?

Correct. Otherwise we'll get some modes and devices that
don't work.

Remember our saga with 74.25 vs 74.40 MHz?

With our PLLs on AXS and HSDK boards we may generate 74.25 MHz clock
which satisfy some monitors especially those who pass correct EDID to the host.
But what if EDID is either corrupted or doesn't exist (that's my case with
some industrial monitor as well as with old DVI monitor)?

In that case Linux kernel attempts to calculate all the values including pixel clock
but then instead of 74.25 we'll get 74.40 and equipment that used to work is no longer useful.

So strictly speaking existing check makes perfect sense. But it reduces
compatibility with not very good monitors.

Probably better solution to the problem is just to throw away [my] faulty HW and
buy equipment that conforms to standards (not really sure if EDID is a hard
requirement for DVI/HDMI displays or this is just an option).

BTW I'm wondering if there're any guidelines on what could be pixel clock
deviation from the requested one?

-Alexey

Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[Jose Abreu]

Hi Alexey,


On 03-03-2017 19:24, Alexey Brodkin wrote:
>
> Correct. Otherwise we'll get some modes and devices that
> don't work.
>
> Remember our saga with 74.25 vs 74.40 MHz?
>
> With our PLLs on AXS and HSDK boards we may generate 74.25 MHz clock
> which satisfy some monitors especially those who pass correct EDID to the host.
> But what if EDID is either corrupted or doesn't exist (that's my case with
> some industrial monitor as well as with old DVI monitor)?
>
> In that case Linux kernel attempts to calculate all the values including pixel clock
> but then instead of 74.25 we'll get 74.40 and equipment that used to work is no longer useful.
>
> So strictly speaking existing check makes perfect sense. But it reduces
> compatibility with not very good monitors.
>
> Probably better solution to the problem is just to throw away [my] faulty HW and
> buy equipment that conforms to standards (not really sure if EDID is a hard
> requirement for DVI/HDMI displays or this is just an option).

Hmm, per DVI and HDMI spec all compliant equipment must support
the EDID. So, either you don't have a compliant equipment or the
EDID is faulty (maybe wrong checksum), if you activate full debug
in drm core you can see the list of probed modes from EDID.

Anyway, per spec they must support EDID, otherwise you won't know
what modes the display support. I think its not very portable to
force a mode because it may not work in some displays. Or, if you
want to make sure it is always works choose 640x480@60 because (I
think) both DVI and HDMI displays must support this mode (though
this would need to change at boot [or at runtime, once AXS PLL
driver is in] the clock frequency from 74 to 25Mhz).

>
> BTW I'm wondering if there're any guidelines on what could be pixel clock
> deviation from the requested one?

The "flavours" of pixel clock are already handled by drm core (in
cea_mode_alternate_clock()) and I think the mode that is passed
to the drivers has always the corrected value (i.e. the value may
vary when you have 60Hz or 59.94Hz, for example, but only the
60Hz value will be passed to the driver).

Best regards,
Jose Miguel Abreu

>
> -Alexey

Re: [PATCH] drm/arcpgu: use .mode_fixup instead of .atomic_check

[Daniel Vetter]

On Fri, Mar 03, 2017 at 06:05:15PM +0000, Jose Abreu wrote:
> Hi Alexey,
> 
> 
> On 03-03-2017 13:27, Alexey Brodkin wrote:
> >
> > So if I understood you correct here what I really need is just to get rid of existing check,
> > right? I.e. the following is to be in v2 respin:
> > ------------------------------->8-------------------------------
> > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > index ad9a95916f1f..86f1555914e8 100644
> > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > @@ -129,20 +129,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
> >                               ~ARCPGU_CTRL_ENABLE_MASK);
> >  }
> >  
> > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> > -                                    struct drm_crtc_state *state)
> > -{
> > -       struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> > -       struct drm_display_mode *mode = &state->adjusted_mode;
> > -       long rate, clk_rate = mode->clock * 1000;
> > -
> > -       rate = clk_round_rate(arcpgu->clk, clk_rate);
> > -       if (rate != clk_rate)
> > -               return -EINVAL;
> > -
> > -       return 0;
> > -}
> > -
> >  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
> >                                       struct drm_crtc_state *state)
> >  {
> > @@ -165,7 +151,6 @@ static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> >         .disable        = arc_pgu_crtc_disable,
> >         .prepare        = arc_pgu_crtc_disable,
> >         .commit         = arc_pgu_crtc_enable,
> > -       .atomic_check   = arc_pgu_crtc_atomic_check,
> >         .atomic_begin   = arc_pgu_crtc_atomic_begin,
> >  };
> > ------------------------------->8-------------------------------
> 
> I don't think you can remove the check entirely as this will make
> any mode be accepted, right?

Yes there's still the issue of atomic_check/mode_fixup vs. mode_valid. I'm
still trying to volunteer someone to fix up that mess. But at least it's
all properly documented I hope (if not, pls send patch).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch