| Message ID | 9ed5af1177ad08c7c2d9c5d9b32ab0154dbd950f.1650435571.git.ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Media freq factor and per-gt enhancements/fixes | expand |
On Wed, 20 Apr 2022 05:17:57 -0700, Andrzej Hajda wrote: > > Hi Ashutosh, Hi Andrzej, > On 20.04.2022 07:21, Ashutosh Dixit wrote: > > All kmalloc'd kobjects need a kobject_put() to free memory. For example in > > previous code, kobj_gt_release() never gets called. The requirement of > > kobject_put() now results in a slightly different code organization. > > > > Cc: Andi Shyti <andi.shyti@intel.com> > > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface") /snip/ > > +void intel_gt_sysfs_unregister(struct intel_gt *gt) > > +{ > > + kobject_put(>->sysfs_gtn); > > +} > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > > index 9471b26752cf..a99aa7e8b01a 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h > > @@ -13,11 +13,6 @@ > > struct intel_gt; > > -struct kobj_gt { > > - struct kobject base; > > - struct intel_gt *gt; > > -}; > > - > > bool is_object_gt(struct kobject *kobj); > > struct drm_i915_private *kobj_to_i915(struct kobject *kobj); > > @@ -28,6 +23,7 @@ intel_gt_create_kobj(struct intel_gt *gt, > > const char *name); > > void intel_gt_sysfs_register(struct intel_gt *gt); > > +void intel_gt_sysfs_unregister(struct intel_gt *gt); > > struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, > > const char *name); > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h > > 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. Thanks. -- Ashutosh
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
On Sun, 24 Apr 2022 15:30:59 -0700, Andi Shyti wrote: > > Hi Ashutosh, > Hi Andi, > [...] > > > -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. I didn't like gtn either. But a sysfs_gt kobject is already part of 'struct drm_i915_private' so I thought I'll put sysfs_gtn (for gt/gtN) in 'struct intel_gt'. Otherwise browsing the code etc. gets confusing. So that was the reason, but I think I'll change the name to 'kobj_gt' (from the current 'sysfs_gtn') in the next rev. That seems better. Thoughts? Will respond to your other comments after posting the next rev patches (now need some rework because Jani doesn't want a dependence between gt and display code in pcode functions). Thanks. -- Ashutosh
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. > > I didn't like gtn either. But a sysfs_gt kobject is already part of 'struct > drm_i915_private' so I thought I'll put sysfs_gtn (for gt/gtN) in 'struct > intel_gt'. Otherwise browsing the code etc. gets confusing. we can even use 'gt_n' if the 'n' is really necessary. Andi
On Wed, 27 Apr 2022 04:45:03 -0700, Andi Shyti wrote: > > Hi Ashutosh, Hi Andi, > > > > -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. > > > > I didn't like gtn either. But a sysfs_gt kobject is already part of 'struct > > drm_i915_private' so I thought I'll put sysfs_gtn (for gt/gtN) in 'struct > > intel_gt'. Otherwise browsing the code etc. gets confusing. > > we can even use 'gt_n' if the 'n' is really necessary. I decided to just go with sysfs_gt in v2 as you had suggested. The total number of instances of sysfs_gt are very few so it didn't seem too bad to have the same member name in the two struct's. Thanks. -- Ashutosh
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index f0014c5072c9..f0c56ca12c0b 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -783,6 +783,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt) { intel_wakeref_t wakeref; + intel_gt_sysfs_unregister(gt); intel_rps_driver_unregister(>->rps); intel_pxp_fini(>->pxp); diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c index 8ec8bc660c8c..6f1b081ca5b7 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -24,7 +24,7 @@ bool is_object_gt(struct kobject *kobj) static struct intel_gt *kobj_to_gt(struct kobject *kobj) { - return container_of(kobj, struct kobj_gt, base)->gt; + return container_of(kobj, struct intel_gt, sysfs_gtn); } struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, @@ -72,21 +72,19 @@ static struct attribute *id_attrs[] = { }; ATTRIBUTE_GROUPS(id); -static void kobj_gt_release(struct kobject *kobj) +/* A kobject needs a release() method even if it does nothing */ +static void kobj_gtn_release(struct kobject *kobj) { - kfree(kobj); } -static struct kobj_type kobj_gt_type = { - .release = kobj_gt_release, +static struct kobj_type kobj_gtn_type = { + .release = kobj_gtn_release, .sysfs_ops = &kobj_sysfs_ops, .default_groups = id_groups, }; void intel_gt_sysfs_register(struct intel_gt *gt) { - struct kobj_gt *kg; - /* * We need to make things right with the * ABI compatibility. The files were originally @@ -98,25 +96,22 @@ void intel_gt_sysfs_register(struct intel_gt *gt) if (gt_is_root(gt)) intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt)); - kg = kzalloc(sizeof(*kg), GFP_KERNEL); - if (!kg) + /* init and xfer ownership to sysfs tree */ + if (kobject_init_and_add(>->sysfs_gtn, &kobj_gtn_type, + gt->i915->sysfs_gt, "gt%d", gt->info.id)) goto exit_fail; - kobject_init(&kg->base, &kobj_gt_type); - kg->gt = gt; - - /* xfer ownership to sysfs tree */ - if (kobject_add(&kg->base, gt->i915->sysfs_gt, "gt%d", gt->info.id)) - goto exit_kobj_put; - - intel_gt_sysfs_pm_init(gt, &kg->base); + intel_gt_sysfs_pm_init(gt, >->sysfs_gtn); return; -exit_kobj_put: - kobject_put(&kg->base); - exit_fail: + kobject_put(>->sysfs_gtn); drm_warn(>->i915->drm, "failed to initialize gt%d sysfs root\n", gt->info.id); } + +void intel_gt_sysfs_unregister(struct intel_gt *gt) +{ + kobject_put(>->sysfs_gtn); +} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h index 9471b26752cf..a99aa7e8b01a 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h @@ -13,11 +13,6 @@ struct intel_gt; -struct kobj_gt { - struct kobject base; - struct intel_gt *gt; -}; - bool is_object_gt(struct kobject *kobj); struct drm_i915_private *kobj_to_i915(struct kobject *kobj); @@ -28,6 +23,7 @@ intel_gt_create_kobj(struct intel_gt *gt, const char *name); void intel_gt_sysfs_register(struct intel_gt *gt); +void intel_gt_sysfs_unregister(struct intel_gt *gt); struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, const char *name); diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h 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; }; enum intel_gt_scratch_field { diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 8521daba212a..3f06106cdcf5 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -259,4 +259,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) device_remove_bin_file(kdev, &dpf_attrs_1); device_remove_bin_file(kdev, &dpf_attrs); + + kobject_put(dev_priv->sysfs_gt); }
All kmalloc'd kobjects need a kobject_put() to free memory. For example in previous code, kobj_gt_release() never gets called. The requirement of kobject_put() now results in a slightly different code organization. Cc: Andi Shyti <andi.shyti@intel.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface") Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt.c | 1 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 35 ++++++++++-------------- drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 6 +--- drivers/gpu/drm/i915/gt/intel_gt_types.h | 3 ++ drivers/gpu/drm/i915/i915_sysfs.c | 2 ++ 5 files changed, 22 insertions(+), 25 deletions(-)