Message-ID: <20190704105653.GB9747@jamwan02-TSP300>
Date:   Thu, 4 Jul 2019 10:57:00 +0000
From:   "james qian wang (Arm Technology China)" <james.qian.wang@....com>
To:     "Lowry Li (Arm Technology China)" <Lowry.Li@....com>,
        Liviu Dudau <Liviu.Dudau@....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 12:01:49PM +0200, Daniel Vetter wrote:
> 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?
>

Sorry, Will use the mode_config->VRR_ENABLED

we use this private property because we're switching to in-tree, before
finish the switch, we still need to maintain our out-of-tree driver which
depend on a older and doesn't have the VRR_ENABLED property. for avoid
diverging the two branch. my old plan is first switch to in-tree, then drop
the out-of-tree driver and then unify the usage.

> > +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

Sorry again.

First I'll send some patches to remove these private properties.

and then discuss for how to impelement them.

The current komeda privates are:

crtc:
   clock_ratio
   slave_planes

plane:
   img_enhancement
   layer_split

Layer_split: it can be deleted and computed in kernel.

img_enhancement:
  it is for image enhancement, can be removed and computed in kernel.
  but I'd like to have it, since it's a seperated function (NOT only
  for scaling or YUV format), I think only user can real know if need
  to enable it.


img_enhancement:
  it is for image enhancement, can be removed and computed in kernel.
  but I'd like to have it, since it's a seperated function (NOT only
  for scaling or YUV format), I think only user can real know if need
  to enable it.
  I think maybe we can add it CORE as an optional drm_plane property.

clock_ratio:
  It's the clock ratio of (main engine lock/output pixel clk) for
  komeda HW's downscaling restriction, as below:

  D71 downscaling must satisfy the following equation

  MCLK                   h_in * v_in
 ------- >= ---------------------------------------------
 PXLCLK     (h_total - (1 + 2 * v_in / v_out)) * v_out

 In only horizontal downscaling situation, the right side should be
 multiplied by (h_total - 3) / (h_active - 3), then equation becomes

  MCLK          h_in
 ------- >= ----------------
  PXLCLK     (h_active - 3)

slave_planes:
  it's not only for the zpos, but most importantly for notify the user
  to group the planes to two resource sets (pipeline-0 resources and pipeline1).
  Per our HW design the two pipelines can be dynamic assigned to CRTC
  according to the usage.
  - like user only enable one CRTC which can use all two pipelines
    (two resource resource sets)
  - but if enabled two CRTCs, only one resource set available for
    each CRTC.

komeda user need to known the clock_ratio and slave_planes, but how
to expose them: private_property, sysfs or other ways, seems we need
to disscuss. :)

Thanks
James

> > +
> > +/** @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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Powered by blists - more mailing lists