From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart 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> References: <3f24db5e699de29357971ce0c23dada803c5fa11.1494347165.git.joabreu@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <3f24db5e699de29357971ce0c23dada803c5fa11.1494347165.git.joabreu@synopsys.com> Sender: linux-kernel-owner@vger.kernel.org To: dri-devel@lists.freedesktop.org Cc: Jose Abreu , linux-kernel@vger.kernel.org, Daniel Vetter , Alexey Brodkin , Carlos Palminha List-Id: dri-devel@lists.freedesktop.org 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. >=20 > 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. >=20 > Also, remove the atomic_check() callback as mode_valid() callback > will be called before. >=20 > Signed-off-by: Jose Abreu > Cc: Carlos Palminha > Cc: Alexey Brodkin > Cc: Ville Syrj=E4l=E4 > 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(-) >=20 > 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 @@ > =09{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB88= 8 }, > }; >=20 > +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,= > +=09=09=09=09 const struct drm_display_mode *mode) > +{ > +=09long rate, clk_rate =3D mode->clock * 1000; > + > +=09rate =3D clk_round_rate(arcpgu->clk, clk_rate); > +=09if (rate !=3D clk_rate) > +=09=09return false; This isn't anything new introduced by this patch, but shouldn't drivers= allow=20 for some margin in clock frequencies ? Surely if the mode requires a=20= 60.000.000 Hz frequency and the hardware can only generate 59.999.999 H= z or=20 60.000.001 Hz we shouldn't fail. As far as I understand, this is someth= ing the=20 mode_fixup() operation is supposed to handle, but the arc driver doesn'= t=20 implement it. > +=09return true; > +} Can't you inline this in arc_pgu_crtc_mode_valid() as there's a single = caller=20 ? > static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) > { > =09struct arcpgu_drm_private *arcpgu =3D crtc_to_arcpgu_priv(crtc); > @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *c= rtc) > =09.atomic_destroy_state =3D drm_atomic_helper_crtc_destroy_state, > }; >=20 > +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc, > +=09=09=09=09=09 const struct drm_display_mode=20 *mode) > +{ > +=09struct arcpgu_drm_private *arcpgu =3D crtc_to_arcpgu_priv(crtc); > + > +=09if (!arc_pgu_is_mode_valid(arcpgu, mode)) > +=09=09return MODE_NOCLOCK; > + > +=09return MODE_OK; > +} > + > static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc) > { > =09struct arcpgu_drm_private *arcpgu =3D crtc_to_arcpgu_priv(crtc); > @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc= *crtc) > ~ARCPGU_CTRL_ENABLE_MASK); > } >=20 > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, > -=09=09=09=09 struct drm_crtc_state *state) > -{ > -=09struct arcpgu_drm_private *arcpgu =3D crtc_to_arcpgu_priv(crtc); > -=09struct drm_display_mode *mode =3D &state->adjusted_mode; > -=09long rate, clk_rate =3D mode->clock * 1000; > - > -=09rate =3D clk_round_rate(arcpgu->clk, clk_rate); > -=09if (rate !=3D clk_rate) > -=09=09return -EINVAL; > - > -=09return 0; > -} > - > static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, > =09=09=09=09 struct drm_crtc_state *state) > { > @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_= crtc > *crtc, } >=20 > static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = =3D { > +=09.mode_valid=09=3D arc_pgu_crtc_mode_valid, > =09.mode_set=09=3D drm_helper_crtc_mode_set, > =09.mode_set_base=09=3D drm_helper_crtc_mode_set_base, > =09.mode_set_nofb=09=3D arc_pgu_crtc_mode_set_nofb, > @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_= crtc > *crtc, .disable=09=3D arc_pgu_crtc_disable, > =09.prepare=09=3D arc_pgu_crtc_disable, > =09.commit=09=09=3D arc_pgu_crtc_enable, > -=09.atomic_check=09=3D arc_pgu_crtc_atomic_check, > =09.atomic_begin=09=3D arc_pgu_crtc_atomic_begin, > }; --=20 Regards, Laurent Pinchart