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:5.010 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site