[Intel-gfx] [RFC v2 2/2] drm/i915/vrr: Attach and set drm crtc vrr_enabled property
This function attaches & sets the vrr_enabled property for crtc based on the platform support and the request from userspace. V2: Check for platform support before updating the prop. Cc: Ville Syrjälä Cc: Manasi Navare Signed-off-by: Bhanuprakash Modem --- drivers/gpu/drm/i915/display/intel_crtc.c | 3 +++ drivers/gpu/drm/i915/display/intel_vrr.c | 8 2 files changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index 4442aa355f86..36deaca9af66 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -366,6 +366,9 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) BIT(DRM_SCALING_FILTER_DEFAULT) | BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR)); + if (HAS_VRR(dev_priv)) + drm_mode_crtc_attach_vrr_enabled_property(&crtc->base); + intel_color_init(crtc); intel_crtc_drrs_init(crtc); diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c index 396f2f994fa0..7263b35550de 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.c +++ b/drivers/gpu/drm/i915/display/intel_vrr.c @@ -160,6 +160,10 @@ void intel_vrr_enable(struct intel_encoder *encoder, enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; u32 trans_vrr_ctl; + if (HAS_VRR(dev_priv)) + drm_mode_crtc_set_vrr_enabled_property(crtc_state->uapi.crtc, + crtc_state->vrr.enable); + if (!crtc_state->vrr.enable) return; @@ -211,6 +215,10 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state) struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder; + if (HAS_VRR(dev_priv)) + drm_mode_crtc_set_vrr_enabled_property(old_crtc_state->uapi.crtc, + false); + if (!old_crtc_state->vrr.enable) return; -- 2.35.1
[Intel-gfx] [RFC v2 1/2] drm/vrr: Attach vrr_enabled property to the drm crtc
Modern display hardware is capable of supporting variable refresh rates.
This patch introduces helpers to attach and set "vrr_enabled" property
on the crtc to allow userspace to query VRR enabled status on that crtc.
Atomic drivers should attach this property to crtcs those are capable of
driving variable refresh rates using
drm_mode_crtc_attach_vrr_enabled_property().
The value should be updated based on driver and hardware capability
by using drm_mode_crtc_set_vrr_enabled_property().
V2: Use property flag as atomic
Cc: Ville Syrjälä
Cc: Nicholas Kazlauskas
Cc: Manasi Navare
Cc: Harry Wentland
Signed-off-by: Bhanuprakash Modem
---
drivers/gpu/drm/drm_crtc.c| 44 +++
drivers/gpu/drm/drm_mode_config.c | 2 +-
include/drm/drm_crtc.h| 4 +++
3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 26a77a735905..95b4a0c7ecb3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -883,3 +883,47 @@ int drm_crtc_create_scaling_filter_property(struct
drm_crtc *crtc,
return 0;
}
EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
+
+/**
+ * drm_mode_crtc_attach_vrr_enabled_property - attaches the vrr_enabled
property
+ * @crtc: drm CRTC to attach the vrr_enabled property on.
+ *
+ * This is used by atomic drivers to add support for querying
+ * VRR enabled status for a crtc.
+ */
+void drm_mode_crtc_attach_vrr_enabled_property(struct drm_crtc *crtc)
+{
+ struct drm_device *dev = crtc->dev;
+ struct drm_mode_config *config = &dev->mode_config;
+
+ if (!config->prop_vrr_enabled)
+ return;
+
+ drm_object_attach_property(&crtc->base,
+ config->prop_vrr_enabled,
+ 0);
+}
+EXPORT_SYMBOL(drm_mode_crtc_attach_vrr_enabled_property);
+
+/**
+ * drm_mode_crtc_set_vrr_enabled_property - sets the vrr enabled property for
+ * a crtc.
+ * @crtc: drm CRTC
+ * @vrr_enabled: True to enable the VRR on CRTC
+ *
+ * Should be used by atomic drivers to update the VRR enabled status on a CRTC
+ */
+void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
+ bool vrr_enabled)
+{
+ struct drm_device *dev = crtc->dev;
+ struct drm_mode_config *config = &dev->mode_config;
+
+ if (!config->prop_vrr_enabled)
+ return;
+
+ drm_object_property_set_value(&crtc->base,
+ config->prop_vrr_enabled,
+ vrr_enabled);
+}
+EXPORT_SYMBOL(drm_mode_crtc_set_vrr_enabled_property);
diff --git a/drivers/gpu/drm/drm_mode_config.c
b/drivers/gpu/drm/drm_mode_config.c
index 37b4b9f0e468..b7cde73d5586 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -323,7 +323,7 @@ static int drm_mode_create_standard_properties(struct
drm_device *dev)
return -ENOMEM;
dev->mode_config.prop_mode_id = prop;
- prop = drm_property_create_bool(dev, 0,
+ prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
"VRR_ENABLED");
if (!prop)
return -ENOMEM;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a70baea0636c..bde657cb0f9e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1333,4 +1333,8 @@ static inline struct drm_crtc *drm_crtc_find(struct
drm_device *dev,
int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
unsigned int supported_filters);
+void drm_mode_crtc_attach_vrr_enabled_property(struct drm_crtc *crtc);
+void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
+ bool vrr_enabled);
+
#endif /* __DRM_CRTC_H__ */
--
2.35.1
[Intel-gfx] [RFC v2 0/2] Attach and Set vrr_enabled property
This series will add a support to attach & set the vrr_enabled property for crtc based on the platform support and the request from userspace. And userspace can also query to get the status of "vrr_enabled". Test-with: 20220422075223.2792586-2-bhanuprakash.mo...@intel.com Bhanuprakash Modem (2): drm/vrr: Attach vrr_enabled property to the drm crtc drm/i915/vrr: Attach and set drm crtc vrr_enabled property drivers/gpu/drm/drm_crtc.c| 44 +++ drivers/gpu/drm/drm_mode_config.c | 2 +- drivers/gpu/drm/i915/display/intel_crtc.c | 3 ++ drivers/gpu/drm/i915/display/intel_vrr.c | 8 + include/drm/drm_crtc.h| 4 +++ 5 files changed, 60 insertions(+), 1 deletion(-) -- 2.35.1
Re: [Intel-gfx] [RFC] drm/i915/rc6: Access RC6 regs with forcewake
On 22-04-2022 21:04, Matt Roper wrote:
On Fri, Apr 22, 2022 at 07:07:52PM +0530, Badal Nilawar wrote:
To access RC6 related MMIO regs use intel_uncore_write(), which grabs
forcewake if reg requires a forcewake.
Signed-off-by: Badal Nilawar
---
drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
b/drivers/gpu/drm/i915/gt/intel_rc6.c
index b4770690e794..9edaad3f19b5 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6.c
+++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
@@ -55,7 +55,7 @@ static struct drm_i915_private *rc6_to_i915(struct intel_rc6
*rc)
static void set(struct intel_uncore *uncore, i915_reg_t reg, u32 val)
{
- intel_uncore_write_fw(uncore, reg, val);
+ intel_uncore_write(uncore, reg, val);
The set() function is primarily used within the *_rc6_enable() functions.
Those are all called via intel_rc6_enable() which has already
grabbed explicit forcewake before calling them:
intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
so there's no need to grab an additional reference on every register
access. Likewise, the call in vlv_residency_raw() doesn't need
forcewake because its own caller (intel_rc6_residency_ns) has already
acquired it.
I think the call in intel_rc6_unpark() is the only one that might be
questionable from a very quick skim of the code. So rather than
needlessly grabbing forcewake in all the other spots, it's probably
better to just replace that single call with a direct call to
intel_uncore_write() if it actually is problematic.
Even better, we might just want to drop the set() wrapper completely and
replace all instances with the appropriate intel_uncore_write[_fw]
calls; I don't really see the slightly shorter lines the wrapper gives
us as being worth the deviation from the rest of the driver's code
(especially if it's causing us confusion about what the intendended
forcewake semantics are).
Thanks for clarification.
The change in set() function was done for inte_rc6_unpark/park() only
since 0xA090 RC6_CTRL is getting written without forcewake. For now as
you suggested in inte_rc6_unpark/park() I will replace set() with
intel_uncore_write().
Regards,
Badal
Matt
}
static void gen11_rc6_enable(struct intel_rc6 *rc6)
--
2.25.1
Re: [Intel-gfx] [PATCH 7/9] drm/i915/gt: Fix memory leaks in per-gt sysfs
Hi Andrzej and Ashutosh,
> > > > b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > index 937b2e1a305e..4c72b4f983a6 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > > > @@ -222,6 +222,9 @@ struct intel_gt {
> > > > } mocs;
> > > > struct intel_pxp pxp;
> > > > +
> > > > + /* gt/gtN sysfs */
> > > > + struct kobject sysfs_gtn;
> > > If you put kobject as a part of intel_gt what assures you that lifetime of
> > > kobject is shorter than intel_gt? Ie its refcounter is 0 on removal of
> > > intel_gt?
> > Because we are explicitly doing a kobject_put() in
> > intel_gt_sysfs_unregister(). Which is exactly what we are *not* doing in
> > the previous code.
> >
> > Let me explain a bit about the previous code (but feel free to skip since
> > the patch should speak for itself):
> > * Previously we kzalloc a 'struct kobj_gt'
> > * But we don't save a pointer to the 'struct kobj_gt' so we don't have the
> >pointer to the kobject to be able to do a kobject_put() on it later
> > * Therefore we need to store the pointer in 'struct intel_gt'
> > * But if we have to put the pointer in 'struct intel_gt' we might as well
> >put the kobject as part of 'struct intel_gt' and that also removes the
> >need to have a 'struct kobj_gt' (kobj_to_gt() can just use container_of()
> >to get gt from kobj).
> > * So I think this patch simpler/cleaner than the original code if you take
> >the requirement for kobject_put() into account.
This is my oversight. This was something I completely forgot to
fix but it was my intention to do and actually I had some fixes
ongoing. But because this patch took too long to get in I
completely forgot about it (Sujaritha was actually the first who
pointed this out).
Thanks, Ashutosh for taking this.
> I fully agree that previous code is incorrect but I am not convinced current
> code is correct.
> If some objects are kref-counted it means usually they can have multiple
> concurrent users and kobject_put does not work as traditional
> destructor/cleanup/unregister.
> So in this particular case after calling kobject_init_and_add sysfs core can
> get multiple references on the object. Later, during driver unregistration
> kobject_put is called, but if the object is still in use by sysfs core, the
> object will not be destroyed/released. If the driver unregistration
> continues memory will be freed, leaving sysfs-core (or other users) with
> dangling pointers. Unless there is some additional synchronization mechanism
> I am not aware of.
Thanks Andrzej for summarizing this and what you said is actually
what happens. I had a similar solution developed and I had wrong
pointer reference happening.
Thanks,
Andi
Re: [Intel-gfx] [PATCH 7/9] drm/i915/gt: Fix memory leaks in per-gt sysfs
Hi Ashutosh,
[...]
> -static struct kobj_type kobj_gt_type = {
> - .release = kobj_gt_release,
> +static struct kobj_type kobj_gtn_type = {
what does it mean GTN? Or is it GTn? Please use just GT, gtn is
confusing.
Same for all the rest of the gtn's you have used below.
Thanks,
Andi
Re: [Intel-gfx] [PATCH 6/9] drm/i915/gt: Add media RP0/RPn to per-gt sysfs
Hi Ashutosh,
[...]
> +static ssize_t media_RP0_freq_mhz_show(struct device *dev,
> +struct device_attribute *attr,
> +char *buff)
> +{
> + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> + u32 val;
> + int err;
> +
> + err = __intel_gt_pcode_read(gt, XEHPSDV_PCODE_FREQUENCY_CONFIG,
> + PCODE_MBOX_FC_SC_READ_FUSED_P0,
> + PCODE_MBOX_DOMAIN_MEDIAFF, &val);
> +
> + if (err)
> + return err;
> +
> + /* data_out - Fused P0 for domain ID in units of 50 MHz */
this comment doesn't say much, can we make it a bit clearer? The
same for the one below.
The rest looks good:
Reviewed-by: Andi Shyti
> + val *= GT_FREQUENCY_MULTIPLIER;
> +
> + return sysfs_emit(buff, "%u\n", val);
> +}
> +
> +static ssize_t media_RPn_freq_mhz_show(struct device *dev,
> +struct device_attribute *attr,
> +char *buff)
> +{
> + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> + u32 val;
> + int err;
> +
> + err = __intel_gt_pcode_read(gt, XEHPSDV_PCODE_FREQUENCY_CONFIG,
> + PCODE_MBOX_FC_SC_READ_FUSED_PN,
> + PCODE_MBOX_DOMAIN_MEDIAFF, &val);
> +
> + if (err)
> + return err;
> +
> + /* data_out - Fused P0 for domain ID in units of 50 MHz */
> + val *= GT_FREQUENCY_MULTIPLIER;
> +
> + return sysfs_emit(buff, "%u\n", val);
> +}
[...]
Thanks,
Andi
Re: [Intel-gfx] [PATCH 5/9] drm/i915/pcode: Add a couple of pcode helpers
Hi Ashutosh, On Tue, Apr 19, 2022 at 11:25:05PM -0700, Ashutosh Dixit wrote: > From: Dale B Stimson > > Add a couple of helpers to help formatting pcode commands and improve code > readability. Can you please add some more details on the helpers? > v2: Fixed commit author (Rodrigo) > > Cc: Mike Ruhl > Cc: Rodrigo Vivi > Signed-off-by: Dale B Stimson > Signed-off-by: Ashutosh Dixit > Reviewed-by: Rodrigo Vivi [...] > +/* > + * Helpers for dGfx PCODE mailbox command formatting > + */ > +int __intel_gt_pcode_read(struct intel_gt *gt, u32 mbcmd, u32 p1, u32 p2, > u32 *val); > +int __intel_gt_pcode_write(struct intel_gt *gt, u32 mbcmd, u32 p1, u32 p2, > u32 val); > + > +#define __snb_pcode_read(i915, mbcmd, p1, p2, val) \ > + __intel_gt_pcode_read(&(i915)->gt0, mbcmd, p1, p2, val) > + > +#define __snb_pcode_write(i915, mbcmd, p1, p2, val) \ > + __intel_gt_pcode_write(&(i915)->gt0, mbcmd, p1, p2, val) to_gt(i915) Why do we need these defines? Looks hacky and lazy. Can't we just replace all __snb_pcode_read/write()? Andi
Re: [Intel-gfx] [PATCH 4/9] drm/i915/gt: Convert callers to user per-gt pcode functions
Hi Ashutosh, On Tue, Apr 19, 2022 at 11:25:04PM -0700, Ashutosh Dixit wrote: > Convert appropriate callers to use per-gt pcode functions. Callers using > pcode functions at "global scope", including *all* display functions are > not converted, they continue to use the legacy pcode interface. > > Cc: Andi Shyti > Cc: Jani Nikula > Cc: Rodrigo Vivi > Signed-off-by: Ashutosh Dixit Reviewed-by: Andi Shyti Thanks, Andi
Re: [Intel-gfx] [PATCH 3/9] drm/i915/pcode: Extend pcode functions for multiple gt's
Hi Ashutosh,
[...]
> -static bool skl_pcode_try_request(struct drm_i915_private *i915, u32 mbox,
> - u32 request, u32 reply_mask, u32 reply,
> - u32 *status)
> +static bool __gt_pcode_try_request(struct intel_gt *gt, u32 mbox,
why is this becoming a '__' function?
> +u32 request, u32 reply_mask, u32 reply,
> +u32 *status)
> {
> - *status = __snb_pcode_rw(i915, mbox, &request, NULL, 500, 0, true);
> + *status = __gt_pcode_rw(gt, mbox, &request, NULL, 500, 0, true);
>
> return (*status == 0) && ((request & reply_mask) == reply);
> }
>
> /**
> - * skl_pcode_request - send PCODE request until acknowledgment
> - * @i915: device private
> + * intel_gt_pcode_request - send PCODE request until acknowledgment
> + * @gt: gt
> * @mbox: PCODE mailbox ID the request is targeted for
> * @request: request ID
> * @reply_mask: mask used to check for request acknowledgment
> @@ -158,16 +159,16 @@ static bool skl_pcode_try_request(struct
> drm_i915_private *i915, u32 mbox,
> * Returns 0 on success, %-ETIMEDOUT in case of a timeout, <0 in case of some
> * other error as reported by PCODE.
> */
> -int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request,
> - u32 reply_mask, u32 reply, int timeout_base_ms)
> +int intel_gt_pcode_request(struct intel_gt *gt, u32 mbox, u32 request,
> +u32 reply_mask, u32 reply, int timeout_base_ms)
> {
> u32 status;
> int ret;
>
> - mutex_lock(&i915->sb_lock);
> + mutex_lock(>->i915->sb_lock);
>
> #define COND \
> - skl_pcode_try_request(i915, mbox, request, reply_mask, reply, &status)
> + __gt_pcode_try_request(gt, mbox, request, reply_mask, reply, &status)
>
> /*
>* Prime the PCODE by doing a request first. Normally it guarantees
> @@ -193,35 +194,48 @@ int skl_pcode_request(struct drm_i915_private *i915,
> u32 mbox, u32 request,
>* requests, and for any quirks of the PCODE firmware that delays
>* the request completion.
>*/
> - drm_dbg_kms(&i915->drm,
> + drm_dbg_kms(>->i915->drm,
> "PCODE timeout, retrying with preemption disabled\n");
> - drm_WARN_ON_ONCE(&i915->drm, timeout_base_ms > 3);
> + drm_WARN_ON_ONCE(>->i915->drm, timeout_base_ms > 3);
> preempt_disable();
> ret = wait_for_atomic(COND, 50);
> preempt_enable();
>
> out:
> - mutex_unlock(&i915->sb_lock);
> + mutex_unlock(>->i915->sb_lock);
> return status ? status : ret;
> #undef COND
> }
>
> +static int __gt_pcode_init(struct intel_gt *gt)
> +{
> + int ret = intel_gt_pcode_request(gt, DG1_PCODE_STATUS,
> + DG1_UNCORE_GET_INIT_STATUS,
> + DG1_UNCORE_INIT_STATUS_COMPLETE,
> + DG1_UNCORE_INIT_STATUS_COMPLETE,
> 18);
> +
> + drm_dbg(>->i915->drm, "gt %d: PCODE init status %d\n", gt->info.id,
> ret);
> +
> + if (ret)
> + drm_err(>->i915->drm, "gt %d: Pcode did not report uncore
> initialization completion!\n",
> + gt->info.id);
> +
> + return ret;
> +}
> +
> int intel_pcode_init(struct drm_i915_private *i915)
> {
> - int ret = 0;
> + struct intel_gt *gt;
> + int i, ret = 0;
>
> if (!IS_DGFX(i915))
> return ret;
we can take some freedom, if you don't mind, and declare ret
inside the for_each, and return 0 here. Just a small cosmetic.
>
> - ret = skl_pcode_request(i915, DG1_PCODE_STATUS,
> - DG1_UNCORE_GET_INIT_STATUS,
> - DG1_UNCORE_INIT_STATUS_COMPLETE,
> - DG1_UNCORE_INIT_STATUS_COMPLETE, 18);
> -
> - drm_dbg(&i915->drm, "PCODE init status %d\n", ret);
> -
> - if (ret)
> - drm_err(&i915->drm, "Pcode did not report uncore initialization
> completion!\n");
> + for_each_gt(gt, i915, i) {
> + ret = __gt_pcode_init(gt);
> + if (ret)
> + return ret;
> + }
>
> - return ret;
> + return 0;
> }
> diff --git a/drivers/gpu/drm/i915/intel_pcode.h
> b/drivers/gpu/drm/i915/intel_pcode.h
> index 0962a17fac48..96c954ec91f9 100644
> --- a/drivers/gpu/drm/i915/intel_pcode.h
> +++ b/drivers/gpu/drm/i915/intel_pcode.h
> @@ -8,16 +8,31 @@
>
> #include
>
> +struct intel_gt;
> struct drm_i915_private;
>
> -int snb_pcode_read(struct drm_i915_private *i915, u32 mbox, u32 *val, u32
> *val1);
> -int snb_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox, u32 val,
> - int fast_timeout_us, int slow_timeout_ms);
> -#define snb_pcode_write(i915, mbox, val) \
> +int intel_gt_pcode_read(struct intel_gt *gt, u32 mbox, u32 *val, u32 *val1);
> +
> +int intel_gt_pcode_write_t
Re: [Intel-gfx] [PATCH 1/9] drm/i915: Introduce has_media_ratio_mode
Hi Ashutosh, > Media ratio mode (the ability for media IP to work at a different frequency > from the GT) is available for a subset of dGfx platforms supporting > GuC/SLPC. Introduce 'has_media_ratio_mode' flag in intel_device_info to > identify these platforms and set it for XEHPSDV and DG2/ATS-M. > > Cc: Rodrigo Vivi > Signed-off-by: Ashutosh Dixit > Reviewed-by: Rodrigo Vivi Reviewed-by: Andi Shyti Thanks, Andi
Re: [Intel-gfx] [PATCH 2/9] drm/i915/gt: Add media freq factor to per-gt sysfs
Hi Ashutosh,
[...]
> static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj,
> const struct attribute * const *attrs)
> {
> @@ -598,4 +720,12 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct
> kobject *kobj)
> drm_warn(>->i915->drm,
>"failed to create gt%u throttle sysfs files (%pe)",
>gt->info.id, ERR_PTR(ret));
> +
> + if (HAS_MEDIA_RATIO_MODE(gt->i915) && intel_uc_uses_guc_slpc(>->uc)) {
you could use in this case the ".is_visible()" function as you are
not inheriting it from the upper drm class.
Anyway,
Reviewed-by: Andi Shyti
Thanks,
Andi
