lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 30 Jul 2018 09:33:55 -0400
From:   Sean Paul <seanpaul@...omium.org>
To:     Satendra Singh Thakur <satendra.t@...sung.com>
Cc:     Gustavo Padovan <gustavo@...ovan.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        vineet.j@...sung.com, hemanshu.s@...sung.com,
        nishant.y08@...sung.com, sst2005@...il.com
Subject: Re: [PATCH] drm/kms/crtc: Saving crtc->primary into a drm_plane
 pointer instead of dereferencing it every time.

On Mon, Jul 30, 2018 at 11:35:58AM +0530, Satendra Singh Thakur wrote:
> In the func __drm_mode_set_config_internal,
> objects (fb, old_fb, crtc) of crtc->primary are used at many places.
> To access the objects of primary, it is dereferenced from crtc every
> time. It's better to save it into drm_plane pointer.
> This will make the code look simple.

Maybe this is simpler, but not overwhelmingly so.

To be honest, I'm a bit concerned with the volume of no-op patches you're
sending to the list. I so appreciate and encourage your participation,
but perhaps we could redirect your efforts. 

A lot of the patches you send (not necessarily this one, it's pretty
straightforward), are changing core pieces of commit validation which have
already been proven to work. These types of changes require a lot of
context and scrutinization on the part of the reviewer, which takes time that
most of us don't have. If you introduce a bug with a no-op change, people are
not going to be happy.

So, in order to make everyone more productive, may I suggest the following:

- Every change you make sure have a purpose greater than "this code snippet
  is marginally more readable".
- If you want to make readability changes, do them in one patch series with
  a common theme (for example, the drm_crtc.h refactors).
- Spend your time on bug fixes, performance improvements, and features as
  opposed to readability.
- Make sure you test your code.

Again, *thank you* for your patches, it's great to have you here.

Sean


> 
> Signed-off-by: Satendra Singh Thakur <satendra.t@...sung.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 98a36e6..9644f5b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -462,6 +462,7 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
>  	struct drm_crtc *crtc = set->crtc;
>  	struct drm_framebuffer *fb;
>  	struct drm_crtc *tmp;
> +	struct drm_plane *plane;
>  	int ret;
>  
>  	/*
> @@ -469,23 +470,27 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
>  	 * connectors from it), hence we need to refcount the fbs across all
>  	 * crtcs. Atomic modeset will have saner semantics ...
>  	 */
> -	drm_for_each_crtc(tmp, crtc->dev)
> -		tmp->primary->old_fb = tmp->primary->fb;
> +	drm_for_each_crtc(tmp, crtc->dev) {
> +		plane = tmp->primary;
> +		plane->old_fb = plane->fb;
> +	}
>  
>  	fb = set->fb;
> -
>  	ret = crtc->funcs->set_config(set, ctx);
>  	if (ret == 0) {
> -		crtc->primary->crtc = fb ? crtc : NULL;
> -		crtc->primary->fb = fb;
> +		plane = crtc->primary;
> +		plane->crtc = fb ? crtc : NULL;
> +		plane->fb = fb;
>  	}
>  
>  	drm_for_each_crtc(tmp, crtc->dev) {
> -		if (tmp->primary->fb)
> -			drm_framebuffer_get(tmp->primary->fb);
> -		if (tmp->primary->old_fb)
> -			drm_framebuffer_put(tmp->primary->old_fb);
> -		tmp->primary->old_fb = NULL;
> +		plane = tmp->primary;
> +		if (plane->fb)
> +			drm_framebuffer_get(plane->fb);
> +		if (plane->old_fb) {
> +			drm_framebuffer_put(plane->old_fb);
> +			plane->old_fb = NULL;
> +		}
>  	}
>  
>  	return ret;
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ