Re: [Freedreno] [PATCH v2 1/4] drm/msm/dpu: add atomic private object to dpu crtc

[Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>]

On 2019-03-04 13:32, Sean Paul wrote:
> On Wed, Feb 13, 2019 at 05:52:19PM -0800, Jeykumar Sankaran wrote:
> > Subclass drm private object state for DPU for handling driver
> > specific data. Adds atomic private object to dpu crtc to
> > track hw resources per display. Provide helper function to
> > retrieve DPU private data from current atomic state before
> > atomic swap.
> >
> > changes in v2:
> > 	- private objects are maintained in dpu_crtc as
> > 	  the resources are tracked per display
>
> Seems like you could just store it in crtc_state if it's per-crtc?
If you mean - "no need for priv object, maintain in crtc state", that
was the original v1 implementation. But you proposed to have them
tracked in a private object since I had to use crtc_state for tracking
interfaces too which were mapped in encoders.

It made sense as the private objects not bounded to any DRM object
domain and is the best candidate to track per display assignments.
So v2 implementation moved all the RM assignment to private objects 
state
and provided an interface for CRTC and Encoder to query its domain
specific hw blocks.

v1: https://patchwork.freedesktop.org/patch/255452/

Thanks and Regards,
Jeykumar S.


> Sean
>
> > Signed-off-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h |  3 ++
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 64
> +++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h  | 15 ++++++++
> >  3 files changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > index e59d62b..be07554 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -141,6 +141,7 @@ struct dpu_crtc_frame_event {
> >   * @frame_pending : Whether or not an update is pending
> >   * @frame_events  : static allocation of in-flight frame events
> >   * @frame_event_list : available frame event list
> > + * @priv_obj	  : private state object to track hw resources
> >   * @spin_lock     : spin lock for frame event, transaction status,
> etc...
> >   * @frame_done_comp    : for frame_event_done synchronization
> >   * @event_thread  : Pointer to event handler thread
> > @@ -176,6 +177,8 @@ struct dpu_crtc {
> >  	spinlock_t spin_lock;
> >  	struct completion frame_done_comp;
> >
> > +	struct drm_private_obj priv_obj;
> > +
> >  	/* for handling internal event thread */
> >  	spinlock_t event_lock;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 885bf88..1677862 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -459,6 +459,49 @@ static int _dpu_kms_setup_displays(struct
> drm_device *dev,
> >  	return _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
> >  }
> >
> > +struct dpu_private_state *dpu_get_private_state(struct 
> > drm_atomic_state
> *state,
> > +						struct dpu_crtc *crtc)
> > +{
> > +	struct drm_private_state *priv_state;
> > +
> > +	priv_state = drm_atomic_get_private_obj_state(state,
> &crtc->priv_obj);
> > +	if (IS_ERR(priv_state))
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	return container_of(priv_state, struct dpu_private_state, base);
> > +}
> > +
> > +static struct drm_private_state *
> > +dpu_private_obj_duplicate_state(struct drm_private_obj *obj)
> > +{
> > +	struct dpu_private_state *dpu_priv_state;
> > +
> > +	dpu_priv_state = kmemdup(obj->state, sizeof(*dpu_priv_state),
> > +				 GFP_KERNEL);
> > +	if (!dpu_priv_state)
> > +		return NULL;
> > +
> > +	__drm_atomic_helper_private_obj_duplicate_state(obj,
> > +
> &dpu_priv_state->base);
> > +
> > +	return &dpu_priv_state->base;
> > +}
> > +
> > +static void dpu_private_obj_destroy_state(struct drm_private_obj 
> > *obj,
> > +				      struct drm_private_state *state)
> > +{
> > +	struct dpu_private_state *dpu_priv_state = container_of(state,
> > +						      struct
> dpu_private_state,
> > +						      base);
> > +
> > +	kfree(dpu_priv_state);
> > +}
> > +
> > +static const struct drm_private_state_funcs priv_obj_funcs = {
> > +	.atomic_duplicate_state = dpu_private_obj_duplicate_state,
> > +	.atomic_destroy_state = dpu_private_obj_destroy_state,
> > +};
> > +
> >  static void _dpu_kms_drm_obj_destroy(struct dpu_kms *dpu_kms)
> >  {
> >  	struct msm_drm_private *priv;
> > @@ -476,8 +519,12 @@ static void _dpu_kms_drm_obj_destroy(struct 
> > dpu_kms
> *dpu_kms)
> >  	}
> >  	priv = dpu_kms->dev->dev_private;
> >
> > -	for (i = 0; i < priv->num_crtcs; i++)
> > +	for (i = 0; i < priv->num_crtcs; i++) {
> > +		struct dpu_crtc *dpu_crtc = to_dpu_crtc(priv->crtcs[i]);
> > +
> > +		drm_atomic_private_obj_fini(&dpu_crtc->priv_obj);
> >  		priv->crtcs[i]->funcs->destroy(priv->crtcs[i]);
> > +	}
> >  	priv->num_crtcs = 0;
> >
> >  	for (i = 0; i < priv->num_planes; i++)
> > @@ -499,6 +546,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> *dpu_kms)
> >  	struct drm_plane *primary_planes[MAX_PLANES], *plane;
> >  	struct drm_plane *cursor_planes[MAX_PLANES] = { NULL };
> >  	struct drm_crtc *crtc;
> > +	struct dpu_private_state *dpu_priv_state;
> >
> >  	struct msm_drm_private *priv;
> >  	struct dpu_mdss_cfg *catalog;
> > @@ -560,11 +608,25 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> *dpu_kms)
> >  	/* Create one CRTC per encoder */
> >  	for (i = 0; i < max_crtc_count; i++) {
> > +		struct dpu_crtc *dpu_crtc;
> > +
> >  		crtc = dpu_crtc_init(dev, primary_planes[i],
> cursor_planes[i]);
> >  		if (IS_ERR(crtc)) {
> >  			ret = PTR_ERR(crtc);
> >  			goto fail;
> >  		}
> > +
> > +		dpu_crtc = to_dpu_crtc(crtc);
> > +
> > +		/* Initialize private obj's */
> > +		dpu_priv_state = kzalloc(sizeof(*dpu_priv_state),
> GFP_KERNEL);
> > +		if (!dpu_priv_state)
> > +			return -ENOMEM;
> > +
> > +		drm_atomic_private_obj_init(&dpu_crtc->priv_obj,
> > +					    &dpu_priv_state->base,
> > +					    &priv_obj_funcs);
> > +
> >  		priv->crtcs[priv->num_crtcs++] = crtc;
> >  	}
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > index ac75cfc..3deedfb 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > @@ -32,6 +32,8 @@
> >  #include "dpu_rm.h"
> >  #include "dpu_core_perf.h"
> >
> > +struct dpu_crtc;
> > +
> >  #define DRMID(x) ((x) ? (x)->base.id : -1)
> >
> >  /**
> > @@ -136,6 +138,10 @@ struct dpu_kms {
> >  	struct dss_module_power mp;
> >  };
> >
> > +struct dpu_private_state {
> > +	struct drm_private_state base;
> > +};
> > +
> >  struct vsync_info {
> >  	u32 frame_count;
> >  	u32 line_count;
> > @@ -143,6 +149,15 @@ struct vsync_info {
> >
> >  #define to_dpu_kms(x) container_of(x, struct dpu_kms, base)
> >
> > +/**
> > + * dpu_get_private_state - get dpu private state from atomic state
> > + * @state: drm atomic state
> > + * @crtc: pointer to crtc obj
> > + * Return: pointer to dpu private state object
> > + */
> > +struct dpu_private_state *dpu_get_private_state(struct 
> > drm_atomic_state
> *state,
> > +						struct dpu_crtc *crtc);
> > +
> >  /* get struct msm_kms * from drm_device * */
> >  #define ddev_to_msm_kms(D) ((D) && (D)->dev_private ? \
> >  		((struct msm_drm_private *)((D)->dev_private))->kms :
> NULL)
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> > a Linux Foundation Collaborative Project
> >
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/freedreno

--
Jeykumar S