lkml.org 
[lkml]   [2017]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 8/8] drm: arc: Use crtc->mode_valid() callback
Date
Hi Jose,

Thank you for the patch.

On Tuesday 09 May 2017 18:00:15 Jose Abreu wrote:
> Now that we have a callback to check if crtc supports a given mode
> we can use it in arcpgu so that we restrict the number of probbed
> modes to the ones we can actually display.
>
> This is specially useful because arcpgu crtc is responsible to set
> a clock value in the commit() stage but unfortunatelly this clock
> does not support all the needed ranges.
>
> Also, remove the atomic_check() callback as mode_valid() callback
> will be called before.
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
> drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++---------------
> 1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c
> b/drivers/gpu/drm/arc/arcpgu_crtc.c index ad9a959..01cae0a 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -32,6 +32,18 @@
> { "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
> };
>
> +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
> + const struct drm_display_mode *mode)
> +{
> + long rate, clk_rate = mode->clock * 1000;
> +
> + rate = clk_round_rate(arcpgu->clk, clk_rate);
> + if (rate != clk_rate)
> + return false;

This isn't anything new introduced by this patch, but shouldn't drivers allow
for some margin in clock frequencies ? Surely if the mode requires a
60.000.000 Hz frequency and the hardware can only generate 59.999.999 Hz or
60.000.001 Hz we shouldn't fail. As far as I understand, this is something the
mode_fixup() operation is supposed to handle, but the arc driver doesn't
implement it.

> + return true;
> +}

Can't you inline this in arc_pgu_crtc_mode_valid() as there's a single caller
?

> static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
> {
> struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
> .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> };
>
> +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
> + const struct drm_display_mode
*mode)
> +{
> + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> +
> + if (!arc_pgu_is_mode_valid(arcpgu, mode))
> + return MODE_NOCLOCK;
> +
> + return MODE_OK;
> +}
> +
> static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
> {
> struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> @@ -129,20 +152,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)
> {
> @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc
> *crtc, }
>
> static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> + .mode_valid = arc_pgu_crtc_mode_valid,
> .mode_set = drm_helper_crtc_mode_set,
> .mode_set_base = drm_helper_crtc_mode_set_base,
> .mode_set_nofb = arc_pgu_crtc_mode_set_nofb,
> @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc
> *crtc, .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,
> };

--
Regards,

Laurent Pinchart

\
 
 \ /
  Last update: 2017-05-12 11:58    [W:0.165 / U:3.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site