Re: [PATCH v3 12/13] drm/msm/dpu: add atomic private object to dpu kms

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

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@codeaurora.org>
>> ---
>>  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.

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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno