Re: [PATCH] drm/kms/crtc: Saving crtc->primary into a drm_plane pointer instead of dereferencing it every time.

[Sean Paul]

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@xxxxxxxxxxx>
> ---
>  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