Message-ID: <20190703100149.GF15868@phenom.ffwll.local>
Date:   Wed, 3 Jul 2019 12:01:49 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     "Lowry Li (Arm Technology China)" <Lowry.Li@....com>
Cc:     Liviu Dudau <Liviu.Dudau@....com>,
        "james qian wang (Arm Technology China)" <james.qian.wang@....com>,
        "maarten.lankhorst@...ux.intel.com" 
        <maarten.lankhorst@...ux.intel.com>,
        "seanpaul@...omium.org" <seanpaul@...omium.org>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        Brian Starkey <Brian.Starkey@....com>,
        Ayan Halder <Ayan.Halder@....com>,
        "Jonathan Chai (Arm Technology China)" <Jonathan.Chai@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "Julien Yin (Arm Technology China)" <Julien.Yin@....com>,
        nd <nd@....com>
Subject: Re: [PATCH] drm/komeda: Adds VRR support

On Wed, Jul 03, 2019 at 07:26:16AM +0000, Lowry Li (Arm Technology China) wrote:
> Adds a new drm property "vrr" and "vrr_enable" and implemented
> the set/get functions, through which userspace could set vfp
> data to komeda.
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@....com>
> ---
>  .../gpu/drm/arm/display/komeda/d71/d71_component.c |  6 +++
>  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c   | 62 ++++++++++++++++++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.h    | 12 +++++
>  .../gpu/drm/arm/display/komeda/komeda_pipeline.h   |  4 +-
>  4 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index ed3f273..c1355f5 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -1065,6 +1065,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
>  				    struct komeda_component_state *state)
>  {
>  	struct drm_crtc_state *crtc_st = state->crtc->state;
> +	struct komeda_crtc_state *kcrtc_st = to_kcrtc_st(crtc_st);
>  	struct drm_display_mode *mode = &crtc_st->adjusted_mode;
>  	u32 __iomem *reg = c->reg;
>  	u32 hactive, hfront_porch, hback_porch, hsync_len;
> @@ -1102,6 +1103,9 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
>  		value |= BS_CTRL_DL;
>  	}
>  
> +	if (kcrtc_st->en_vrr)
> +		malidp_write32_mask(reg, BS_VINTERVALS, 0x3FFF, kcrtc_st->vfp);
> +
>  	malidp_write32(reg, BLK_CONTROL, value);
>  }
>  
> @@ -1171,6 +1175,8 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
>  	ctrlr = to_ctrlr(c);
>  
>  	ctrlr->supports_dual_link = true;
> +	ctrlr->supports_vrr = true;
> +	set_range(&ctrlr->vfp_range, 0, 0x3FF);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 4f580b0..3744e6d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -467,6 +467,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
>  
>  	state = kzalloc(sizeof(*state), GFP_KERNEL);
>  	if (state) {
> +		state->vfp = 0;
> +		state->en_vrr = 0;
>  		crtc->state = &state->base;
>  		crtc->state->crtc = crtc;
>  	}
> @@ -487,6 +489,8 @@ static void komeda_crtc_reset(struct drm_crtc *crtc)
>  	new->affected_pipes = old->active_pipes;
>  	new->clock_ratio = old->clock_ratio;
>  	new->max_slave_zorder = old->max_slave_zorder;
> +	new->vfp = old->vfp;
> +	new->en_vrr = old->en_vrr;
>  
>  	return &new->base;
>  }
> @@ -525,6 +529,30 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
>  
>  	if (property == kcrtc->clock_ratio_property) {
>  		*val = kcrtc_st->clock_ratio;
> +	} else if (property == kcrtc->vrr_property) {
> +		*val = kcrtc_st->vfp;
> +	} else if (property == kcrtc->vrr_enable_property) {
> +		*val = kcrtc_st->en_vrr;
> +	} else {
> +		DRM_DEBUG_DRIVER("Unknown property %s\n", property->name);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int komeda_crtc_atomic_set_property(struct drm_crtc *crtc,
> +					   struct drm_crtc_state *state,
> +					   struct drm_property *property,
> +					   uint64_t val)
> +{
> +	struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> +	struct komeda_crtc_state *kcrtc_st = to_kcrtc_st(state);
> +
> +	if (property == kcrtc->vrr_property) {
> +		kcrtc_st->vfp = val;
> +	} else if (property == kcrtc->vrr_enable_property) {
> +		kcrtc_st->en_vrr = val;
>  	} else {
>  		DRM_DEBUG_DRIVER("Unknown property %s\n", property->name);
>  		return -EINVAL;
> @@ -544,6 +572,7 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
>  	.enable_vblank		= komeda_crtc_vblank_enable,
>  	.disable_vblank		= komeda_crtc_vblank_disable,
>  	.atomic_get_property	= komeda_crtc_atomic_get_property,
> +	.atomic_set_property	= komeda_crtc_atomic_set_property,
>  };
>  
>  int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
> @@ -613,6 +642,35 @@ static int komeda_crtc_create_slave_planes_property(struct komeda_crtc *kcrtc)
>  	return 0;
>  }
>  
> +static int komeda_crtc_create_vrr_property(struct komeda_crtc *kcrtc)
> +{
> +	struct drm_crtc *crtc = &kcrtc->base;
> +	struct drm_property *prop;
> +	struct komeda_timing_ctrlr *ctrlr = kcrtc->master->ctrlr;
> +
> +	if (!ctrlr->supports_vrr)
> +		return 0;
> +
> +	prop = drm_property_create_range(crtc->dev, DRM_MODE_PROP_ATOMIC, "vrr",
> +					 ctrlr->vfp_range.start,
> +					 ctrlr->vfp_range.end);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&crtc->base, prop, 0);
> +	kcrtc->vrr_property = prop;
> +
> +	prop = drm_property_create_bool(crtc->dev, DRM_MODE_PROP_ATOMIC,
> +					"enable_vrr");

Uh, what exactly are you doing reinventing uapi properties that we already
standardized?

> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&crtc->base, prop, 0);
> +	kcrtc->vrr_enable_property = prop;
> +
> +	return 0;
> +}
> +
>  static struct drm_plane *
>  get_crtc_primary(struct komeda_kms_dev *kms, struct komeda_crtc *crtc)
>  {
> @@ -659,6 +717,10 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>  	if (err)
>  		return err;
>  
> +	err = komeda_crtc_create_vrr_property(kcrtc);
> +	if (err)
> +		return err;
> +
>  	return err;
>  }
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index dc1d436..d0cf838 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -98,6 +98,12 @@ struct komeda_crtc {
>  
>  	/** @slave_planes_property: property for slaves of the planes */
>  	struct drm_property *slave_planes_property;

And this seems to not be the first time this happened. Looking at komeda
with a quick git grep on properties you've actually accumulated quite a
pile of such driver properties already. Where's the userspace for this?
Where's the uapi discussions for this stuff? Where's the igt tests for
this (yes a bunch are after we agreed to have testcases for this).

I know that in the past we've been somewhat sloppy properties, but that
was a mistake and we've cranked down on this hard. Probably need to fix
this with a pile of reverts and start over.
-Daniel

> +
> +	/** @vrr_property: property for variable refresh rate */
> +	struct drm_property *vrr_property;
> +
> +	/** @vrr_enable_property: property for enable/disable the vrr */
> +	struct drm_property *vrr_enable_property;
>  };
>  
>  /**
> @@ -126,6 +132,12 @@ struct komeda_crtc_state {
>  
>  	/** @max_slave_zorder: the maximum of slave zorder */
>  	u32 max_slave_zorder;
> +
> +	/** @vfp: the value of vertical front porch */
> +	u32 vfp;
> +
> +	/** @en_vrr: enable status of variable refresh rate */
> +	u8 en_vrr : 1;
>  };
>  
>  /** struct komeda_kms_dev - for gather KMS related things */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index 00e8083..66d7664 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -336,7 +336,9 @@ struct komeda_improc_state {
>  /* display timing controller */
>  struct komeda_timing_ctrlr {
>  	struct komeda_component base;
> -	u8 supports_dual_link : 1;
> +	u8 supports_dual_link : 1,
> +	   supports_vrr : 1;
> +	struct malidp_range vfp_range;
>  };
>  
>  struct komeda_timing_ctrlr_state {
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists