On Tue, Aug 14, 2018 at 05:38:31PM -0700, Jeykumar Sankaran wrote: > On 2018-08-14 13:26, Sean Paul wrote: > > On Tue, Aug 07, 2018 at 08:20:10PM -0700, Jeykumar Sankaran wrote: > > > Subclass drm private state for DPU for handling driver > > > specific data. Adds atomic private object and private object > > > lock to dpu kms. Provides helper function to retrieve DPU > > > private data from current atomic state. > > > > > > changes in v2: > > > - none > > > changes in v3: > > > - rebase on [1] > > > > > > [1] > > > https://gitlab.freedesktop.org/seanpaul/dpu-staging/commits/for-next > > > > > > Change-Id: Iaab32badff224ffed024e6ef6576efc8b3af3aec > > > Signed-off-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx> > > > --- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 61 > > +++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 15 ++++++++ > > > 2 files changed, 76 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > > index 7dd6bd2..5e87b9d 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > > @@ -1168,10 +1168,59 @@ static int dpu_kms_hw_init(struct msm_kms > > > *kms) > > > return rc; > > > } > > > > > > +struct dpu_private_state *dpu_get_private_state(struct > > > drm_atomic_state > > *state) > > > +{ > > > + struct msm_drm_private *priv = state->dev->dev_private; > > > + struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms); > > > + struct drm_private_state *priv_state; > > > + int rc = 0; > > > + > > > + rc = drm_modeset_lock(&dpu_kms->priv_obj_lock, > > state->acquire_ctx); > > > + if (rc) > > > + return ERR_PTR(rc); > > > + > > > + priv_state = drm_atomic_get_private_obj_state(state, > > > + &dpu_kms->priv_obj); > > > + if (IS_ERR(priv_state)) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + return to_dpu_private_state(priv_state); > > > +} > > > + > > > +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 = > > to_dpu_private_state(state); > > > + > > > + 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, > > > +}; > > > + > > > > All of this copypasta between mdp5 and dpu is pretty icky. Can we do a > > better > > job of sharing code? Perhaps some helpers in msm_atomic to help manage > > the > > priv_obj? > > > > > struct msm_kms *dpu_kms_init(struct drm_device *dev) > > > { > > > struct msm_drm_private *priv; > > > struct dpu_kms *dpu_kms; > > > + struct dpu_private_state *dpu_priv_state; > > > int irq; > > > > > > if (!dev || !dev->dev_private) { > > > @@ -1189,6 +1238,18 @@ struct msm_kms *dpu_kms_init(struct drm_device > > *dev) > > > } > > > dpu_kms->base.irq = irq; > > > > > > + /* Initialize private obj's */ > > > + drm_modeset_lock_init(&dpu_kms->priv_obj_lock); > > > + > > > + dpu_priv_state = kzalloc(sizeof(*dpu_priv_state), GFP_KERNEL); > > > + if (!dpu_priv_state) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + > > > + drm_atomic_private_obj_init(&dpu_kms->priv_obj, > > > + &dpu_priv_state->base, > > > + &priv_obj_funcs); > > > + > > > return &dpu_kms->base; > > > } > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > > index 66d4666..2579c983 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > > @@ -145,6 +145,9 @@ struct dpu_kms { > > > struct dpu_hw_vbif *hw_vbif[VBIF_MAX]; > > > struct dpu_hw_mdp *hw_mdp; > > > > > > + struct drm_modeset_lock priv_obj_lock; > > > + struct drm_private_obj priv_obj; > > > + > > > bool has_danger_ctrl; > > > > > > struct platform_device *pdev; > > > @@ -152,12 +155,24 @@ 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; > > > }; > > > > > > #define to_dpu_kms(x) container_of(x, struct dpu_kms, base) > > > +#define to_dpu_private_state(x) container_of(x, struct > > dpu_private_state, base) > > > > Do we really need this? It seems like we shouldn't have _that_ many > > structs > > containing dpu_private_state that we need the generic macro. > > > Now that resource manager is the only obj being tracked by the private > state, only > CRTC and Encoder are using this macro to retrieve the dpu_private_state > objects. > But going forward, when DPU starts supporting other value-added hw blocks > (e.g. Post > processing), we need to track more states. So I thought it would come in > handy. Ehh, let's not make it too easy to proliferate private state subclasses. Let's use strongly typed static inlines instead. Sean > > Jeykumar S. > > > > + > > > +/** > > > + * dpu_get_private_state - get dpu private state from atomic state > > > + * @state: drm atomic state > > > + * Return: pointer to dpu private state object > > > + */ > > > +struct dpu_private_state *dpu_get_private_state(struct > > > drm_atomic_state > > *state); > > > > > > /* get struct msm_kms * from drm_device * */ > > > #define ddev_to_msm_kms(D) ((D) && (D)->dev_private ? \ > > > -- > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > > Forum, > > > a Linux Foundation Collaborative Project > > > > > -- > Jeykumar S -- Sean Paul, Software Engineer, Google / Chromium OS
