From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757678AbdELJ5b (ORCPT ); Fri, 12 May 2017 05:57:31 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:36443 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756524AbdELJ53 (ORCPT ); Fri, 12 May 2017 05:57:29 -0400 From: Laurent Pinchart To: dri-devel@lists.freedesktop.org Cc: Jose Abreu , linux-kernel@vger.kernel.org, Daniel Vetter , Alexey Brodkin , Carlos Palminha Subject: Re: [PATCH v2 8/8] drm: arc: Use crtc->mode_valid() callback Date: Fri, 12 May 2017 12:57:30 +0300 Message-ID: <48215037.0gN33EkSK6@avalon> User-Agent: KMail/4.14.10 (Linux/4.9.16-gentoo; KDE/4.14.29; x86_64; ; ) In-Reply-To: <3f24db5e699de29357971ce0c23dada803c5fa11.1494347165.git.joabreu@synopsys.com> References: <3f24db5e699de29357971ce0c23dada803c5fa11.1494347165.git.joabreu@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v4C9vY5i031433 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 > Cc: Carlos Palminha > Cc: Alexey Brodkin > Cc: Ville Syrjälä > Cc: Daniel Vetter > Cc: Dave Airlie > Cc: Andrzej Hajda > Cc: Archit Taneja > --- > 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