Re: [DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources

From: Sean Paul <seanpaul@chromium.org>
To: Jeykumar Sankaran <jsanka@codeaurora.org>
Cc: linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	hoegsberg@google.com, freedreno@lists.freedesktop.org
Subject: Re: [DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources
Date: Thu, 14 Jun 2018 11:49:49 -0400	[thread overview]
Message-ID: <20180614154949.GE20303@art_vandelay> (raw)
In-Reply-To: <1528852667-14833-5-git-send-email-jsanka@codeaurora.org>

On Tue, Jun 12, 2018 at 06:17:47PM -0700, Jeykumar Sankaran wrote:
> Switch to state based resource management. This patch
> overhauls the resource manager and HW allocation methods by
> maintaining the global resource pool and allocated hw
> blocks in respective drm component states.
> 
> Global resource manager(RM) is tracked in private object.
> Allocation strategy is switched from single point allocation
> of HW resources for the display pipeline to per component
> based allocation, where each drm component allocates HW
> blocks mapped to it's domain and tracks them in their respective
> state objects.
> 
> Fixes resource contention due to race conditions between
> user space and display thread by reserving resources
> only in atomic check.
> 
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           | 210 +++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h           |  59 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 223 ++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |   4 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |   9 +-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  32 +-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  86 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |  19 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h            |   8 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 805 ++++++---------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             | 149 ++--
>  11 files changed, 534 insertions(+), 1070 deletions(-)

Ok, there's a lot going on here. It's pretty easy to review megapatches where
the diffstat is mostly negative. However, this patch has a lot of code deleted
and moving around, along with the new private obj. It's really hard to review
changes like this.

Could you please split this up into a bunch of simple patches which do one
thing? ie: Moving topology is a patch, using cstate instead of crtc is a patch,
using private obj is a patch, etc, etc.

Basically, cut things down into small enough pieces such that each patch can
be easily explained without using "and" in the commit message :-)

/snip

> +
>  	dpu_crtc = to_dpu_crtc(crtc);
>  	cstate = to_dpu_crtc_state(crtc->state);
>  	mode = &cstate->base.adjusted_mode;
>  	priv = crtc->dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
> +
> +	/* accessing after swap state. piv_obj.state is the current state */

s/piv_obj/priv_obj/

> +	dpu_priv_state = to_dpu_private_state(dpu_kms->priv_obj.state);
>  
>  	DPU_DEBUG("crtc%d\n", crtc->base.id);
>  

/snip

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel