From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Hunter Subject: Re: [PATCH 2/3] drm: replace the 'for' condition with outside defined variable Date: Tue, 17 Mar 2015 17:29:25 +0800 Message-ID: References: <1426577428-30560-1-git-send-email-zhaojunwang@pku.edu.cn> <1426577428-30560-2-git-send-email-zhaojunwang@pku.edu.cn> <20150317084013.GO21993@phenom.ffwll.local> <20150317092410.GP21993@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1035344611==" Return-path: Received: from mail-ob0-f170.google.com (mail-ob0-f170.google.com [209.85.214.170]) by gabe.freedesktop.org (Postfix) with ESMTP id 6ED0389938 for ; Tue, 17 Mar 2015 02:29:26 -0700 (PDT) Received: by obbgg8 with SMTP id gg8so2829141obb.1 for ; Tue, 17 Mar 2015 02:29:26 -0700 (PDT) In-Reply-To: <20150317092410.GP21993@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org --===============1035344611== Content-Type: multipart/alternative; boundary=001a1141c7164bd4940511789a08 --001a1141c7164bd4940511789a08 Content-Type: text/plain; charset=UTF-8 Got it! Cheers, John On Tue, Mar 17, 2015 at 5:24 PM, Daniel Vetter wrote: > On Tue, Mar 17, 2015 at 04:48:23PM +0800, John Hunter wrote: > > Hi Daniel, > > > > On Tue, Mar 17, 2015 at 4:40 PM, Daniel Vetter wrote: > > > > > On Tue, Mar 17, 2015 at 03:30:27PM +0800, John Hunter wrote: > > > > use outdise defined variable can reduce the recaculate of the > > > > count of planes, crtcs and connectors. > > > > > > > > Signed-off-by: John Hunter > > > > > > Hm, what's the benefit you see for this change? The lines aren't too > long > > > yet and we don't reuse the expression, so imo code readability isn't > > > improved. > > > > > I change this just reference some other functions in the same file. > > like, > > drm_atomic_helper_check_planes > > wait_for_fences > > ... > > I really think we should keep the same coding style in the same file. > > If I am wrong with that, just ignore this patch :-) > > Indeed that's a bit inconsistent. But in cases like these where neither > approach is really better I usually go with "don't change anything". Btw > for the next patch the above explanation should be in the commit message. > The important part isn't really explaining what you change (the code > should be readable enough to make that clear), but _why_ you change > something. > -Daniel > > > > > > -Daniel > > > > --- > > > > drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++--- > > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > b/drivers/gpu/drm/drm_atomic_helper.c > > > > index 39369ee..20376e6 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > @@ -1301,8 +1301,11 @@ void drm_atomic_helper_swap_state(struct > > > drm_device *dev, > > > > struct drm_atomic_state *state) > > > > { > > > > int i; > > > > + int nconnectors = dev->mode_config.num_connector; > > > > + int ncrtcs = dev->mode_config.num_crtc; > > > > + int nplanes = dev->mode_config.num_total_plane; > > > > > > > > - for (i = 0; i < dev->mode_config.num_connector; i++) { > > > > + for (i = 0; i < nconnectors; i++) { > > > > struct drm_connector *connector = state->connectors[i]; > > > > > > > > if (!connector) > > > > @@ -1313,7 +1316,7 @@ void drm_atomic_helper_swap_state(struct > > > drm_device *dev, > > > > connector->state->state = NULL; > > > > } > > > > > > > > - for (i = 0; i < dev->mode_config.num_crtc; i++) { > > > > + for (i = 0; i < ncrtcs; i++) { > > > > struct drm_crtc *crtc = state->crtcs[i]; > > > > > > > > if (!crtc) > > > > @@ -1324,7 +1327,7 @@ void drm_atomic_helper_swap_state(struct > > > drm_device *dev, > > > > crtc->state->state = NULL; > > > > } > > > > > > > > - for (i = 0; i < dev->mode_config.num_total_plane; i++) { > > > > + for (i = 0; i < nplanes; i++) { > > > > struct drm_plane *plane = state->planes[i]; > > > > > > > > if (!plane) > > > > -- > > > > 1.9.1 > > > > > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > > > > -- > > Best regards > > Junwang Zhao > > Microprocessor Research and Develop Center > > Department of Computer Science &Technology > > Peking University > > Beijing, 100871, PRC > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > -- Best regards Junwang Zhao Microprocessor Research and Develop Center Department of Computer Science &Technology Peking University Beijing, 100871, PRC --001a1141c7164bd4940511789a08 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Got it!

Cheers,
John

On Tue, Mar 17= , 2015 at 5:24 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">On Tue, Mar 17, 2015 at 04:4= 8:23PM +0800, John Hunter wrote:
> Hi Daniel,
>
> On Tue, Mar 17, 2015 at 4:40 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Tue, Mar 17, 2015 at 03:30:27PM +0800, John Hunter wrote:
> > > use outdise defined variable can reduce the recaculate of th= e
> > > count of planes, crtcs and connectors.
> > >
> > > Signed-off-by: John Hunter <zhjwpku@gmail.com>
> >
> > Hm, what's the benefit you see for this change? The lines are= n't too long
> > yet and we don't reuse the expression, so imo code readabilit= y isn't
> > improved.
> >
> I change this just reference some other functions in the same file. > like,
>=C2=A0 =C2=A0 =C2=A0 drm_atomic_helper_check_planes
>=C2=A0 =C2=A0 =C2=A0 wait_for_fences
>=C2=A0 =C2=A0 =C2=A0 ...
> I really think we should keep the same coding style in the same file.<= br> > If I am wrong with that, just ignore this patch :-)

Indeed that's a bit inconsistent. But in cases like these where = neither
approach is really better I usually go with "don't change anything= ". Btw
for the next patch the above explanation should be in the commit message. The important part isn't really explaining what you change (the code should be readable enough to make that clear), but _why_ you change
something.
-Daniel

>
> > -Daniel
> > > ---
> > >=C2=A0 drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++---
> > >=C2=A0 1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 39369ee..20376e6 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1301,8 +1301,11 @@ void drm_atomic_helper_swap_state(str= uct
> > drm_device *dev,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_atomi= c_state *state)
> > >=C2=A0 {
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0int i;
> > > +=C2=A0 =C2=A0 =C2=A0int nconnectors =3D dev->mode_config= .num_connector;
> > > +=C2=A0 =C2=A0 =C2=A0int ncrtcs =3D dev->mode_config.num_= crtc;
> > > +=C2=A0 =C2=A0 =C2=A0int nplanes =3D dev->mode_config.num= _total_plane;
> > >
> > > -=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < dev->mode_confi= g.num_connector; i++) {
> > > +=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < nconnectors; i++) = {
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct= drm_connector *connector =3D state->connectors[i];
> > >
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!c= onnector)
> > > @@ -1313,7 +1316,7 @@ void drm_atomic_helper_swap_state(stru= ct
> > drm_device *dev,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0connec= tor->state->state =3D NULL;
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> > >
> > > -=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < dev->mode_confi= g.num_crtc; i++) {
> > > +=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < ncrtcs; i++) {
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct= drm_crtc *crtc =3D state->crtcs[i];
> > >
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!c= rtc)
> > > @@ -1324,7 +1327,7 @@ void drm_atomic_helper_swap_state(stru= ct
> > drm_device *dev,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0crtc-&= gt;state->state =3D NULL;
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> > >
> > > -=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < dev->mode_confi= g.num_total_plane; i++) {
> > > +=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < nplanes; i++) { > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct= drm_plane *plane =3D state->planes[i];
> > >
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!p= lane)
> > > --
> > > 1.9.1
> > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel= @lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/dri= -devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ff= wll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@list= s.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-deve= l
> >
>
>
>
> --
> Best regards
> Junwang Zhao
> Microprocessor Research and Develop Center
> Department of Computer Science &Technology
> Peking University
> Beijing, 100871, PRC

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



--
=
Best regards
=
Junwang Zhao
Microprocessor Research and Develop Center
Department of Computer Science &Technology
Peking Unive= rsity
Beijing, 100871, PRC
--001a1141c7164bd4940511789a08-- --===============1035344611== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1035344611==--