| Message ID | 20221206092100.274195-5-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: Update GuC relay logging debugfs | expand |
On Tue, 06 Dec 2022 01:20:59 -0800, Alan Previn wrote: > > GuC log relay debugfs name for the control handle vs the actual relay > channel are vague. Fix them so it's obvious from the name. No real objection to anything here, just a couple of questions. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 2 +- > .../drm/i915/gt/uc/intel_guc_log_debugfs.c | 22 +++++++++---------- > 2 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > index 6e880d9f42d4..d019c60d34e8 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > @@ -551,7 +551,7 @@ static int guc_log_relay_create(struct intel_guc_log *log) > */ > n_subbufs = intel_guc_log_relay_subbuf_count(log); > > - guc_log_relay_chan = relay_open("guc_log", > + guc_log_relay_chan = relay_open("guc_log_relay_chan", Is this a user visible name or just something internal? I.e. Is this a user visible file name? > dev_priv->drm.primary->debugfs_root, > subbuf_size, n_subbufs, > &relay_callbacks, dev_priv); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c > index 27756640338e..feff1e606b38 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c > @@ -137,7 +137,7 @@ DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_subbuf_count_fops, > guc_log_relay_subbuf_count_get, NULL, > "%lld\n"); > > -static int guc_log_relay_open(struct inode *inode, struct file *file) > +static int guc_log_relay_ctl_open(struct inode *inode, struct file *file) Again not objecting, but what is the purpose/thinking behind adding _ctl_ to these function names? The previous names seemed fine? > { > struct intel_guc_log *log = inode->i_private; > > @@ -150,10 +150,10 @@ static int guc_log_relay_open(struct inode *inode, struct file *file) > } > > static ssize_t > -guc_log_relay_write(struct file *filp, > - const char __user *ubuf, > - size_t cnt, > - loff_t *ppos) > +guc_log_relay_ctl_write(struct file *filp, > + const char __user *ubuf, > + size_t cnt, > + loff_t *ppos) > { > struct intel_guc_log *log = filp->private_data; > int val; > @@ -175,7 +175,7 @@ guc_log_relay_write(struct file *filp, > return ret ?: cnt; > } > > -static int guc_log_relay_release(struct inode *inode, struct file *file) > +static int guc_log_relay_ctl_release(struct inode *inode, struct file *file) > { > struct intel_guc_log *log = inode->i_private; > > @@ -183,11 +183,11 @@ static int guc_log_relay_release(struct inode *inode, struct file *file) > return 0; > } > > -static const struct file_operations guc_log_relay_fops = { > +static const struct file_operations guc_log_relay_ctl_fops = { > .owner = THIS_MODULE, > - .open = guc_log_relay_open, > - .write = guc_log_relay_write, > - .release = guc_log_relay_release, > + .open = guc_log_relay_ctl_open, > + .write = guc_log_relay_ctl_write, > + .release = guc_log_relay_ctl_release, > }; > > void intel_guc_log_debugfs_register(struct intel_guc_log *log, > @@ -197,7 +197,7 @@ void intel_guc_log_debugfs_register(struct intel_guc_log *log, > { "guc_log_dump", &guc_log_dump_fops, NULL }, > { "guc_load_err_log_dump", &guc_load_err_log_dump_fops, NULL }, > { "guc_log_level", &guc_log_level_fops, NULL }, > - { "guc_log_relay", &guc_log_relay_fops, NULL }, > + { "guc_log_relay_ctl", &guc_log_relay_ctl_fops, NULL }, > { "guc_log_relay_subbuf_size", &guc_log_relay_subbuf_size_fops, NULL }, > { "guc_log_relay_subbuf_count", &guc_log_relay_subbuf_count_fops, NULL }, > }; Thanks. -- Ashutosh
Again, only now having time to relook at this. Will re-rev and reconnect with Ashutosh offline since I've been absent on this for too long. Thanks again Ashutosh for reviewing all these back in May. On Wed, 2022-12-07 at 08:50 -0800, Dixit, Ashutosh wrote: > On Tue, 06 Dec 2022 01:20:59 -0800, Alan Previn wrote: > > > > GuC log relay debugfs name for the control handle vs the actual relay > > channel are vague. Fix them so it's obvious from the name. > > No real objection to anything here, just a couple of questions. > > > > > alan:snip > > n_subbufs = intel_guc_log_relay_subbuf_count(log); > > > > - guc_log_relay_chan = relay_open("guc_log", > > + guc_log_relay_chan = relay_open("guc_log_relay_chan", > > Is this a user visible name or just something internal? I.e. Is this a user > visible file name? > It will be user visible. I think we need sudo though - need to double check that. And in case i missed it earlier, the name change was solely to differentiate the "relay channel" handle (that needs to be openned and read as per kernel relay- channel framework) from all the other guc-log related debug-fs. alan:snip > > @@ -137,7 +137,7 @@ DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_subbuf_count_fops, > > guc_log_relay_subbuf_count_get, NULL, > > "%lld\n"); > > > > -static int guc_log_relay_open(struct inode *inode, struct file *file) > > +static int guc_log_relay_ctl_open(struct inode *inode, struct file *file) > > Again not objecting, but what is the purpose/thinking behind adding _ctl_ > to these function names? The previous names seemed fine? > Nothing wrong with the previous one - but since the existing relay logging tool never worked anyways, i figure why not change the name to include "ctl" since we are already using it for the tool to trigger flush by writing '1' to it,... if in future we ever need more controls like 'write 2 for something else' or 'write 3 for something else' (i can think of a few examples but nothing urgent that needs to be part of this immediate series). I'm okay with changing back to original name - but for now will assume this new name is okay - will connect offline.
> > > -static int guc_log_relay_open(struct inode *inode, struct file *file) > > > +static int guc_log_relay_ctl_open(struct inode *inode, struct file *file) > > > > Again not objecting, but what is the purpose/thinking behind adding _ctl_ > > to these function names? The previous names seemed fine? > > > Nothing wrong with the previous one - but since the existing relay logging tool > never worked anyways, i figure why not change the name to include "ctl" since we > are already using it for the tool to trigger flush by writing '1' to it,... if in > future we ever need more controls like 'write 2 for something else' or 'write 3 > for something else' (i can think of a few examples but nothing urgent that needs to > be part of this immediate series). > > I'm okay with changing back to original name - but for now will assume this new name > is okay - will connect offline. > Alan: I did want to also raise the point that this series also gets all the function and debufs names to align with "guc_log_relay_[function/data"] That is occuring across all the new handles i have added and why i am changing some of the old ones like the above "guc_log_relay_ctl"
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c index 6e880d9f42d4..d019c60d34e8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c @@ -551,7 +551,7 @@ static int guc_log_relay_create(struct intel_guc_log *log) */ n_subbufs = intel_guc_log_relay_subbuf_count(log); - guc_log_relay_chan = relay_open("guc_log", + guc_log_relay_chan = relay_open("guc_log_relay_chan", dev_priv->drm.primary->debugfs_root, subbuf_size, n_subbufs, &relay_callbacks, dev_priv); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c index 27756640338e..feff1e606b38 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c @@ -137,7 +137,7 @@ DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_subbuf_count_fops, guc_log_relay_subbuf_count_get, NULL, "%lld\n"); -static int guc_log_relay_open(struct inode *inode, struct file *file) +static int guc_log_relay_ctl_open(struct inode *inode, struct file *file) { struct intel_guc_log *log = inode->i_private; @@ -150,10 +150,10 @@ static int guc_log_relay_open(struct inode *inode, struct file *file) } static ssize_t -guc_log_relay_write(struct file *filp, - const char __user *ubuf, - size_t cnt, - loff_t *ppos) +guc_log_relay_ctl_write(struct file *filp, + const char __user *ubuf, + size_t cnt, + loff_t *ppos) { struct intel_guc_log *log = filp->private_data; int val; @@ -175,7 +175,7 @@ guc_log_relay_write(struct file *filp, return ret ?: cnt; } -static int guc_log_relay_release(struct inode *inode, struct file *file) +static int guc_log_relay_ctl_release(struct inode *inode, struct file *file) { struct intel_guc_log *log = inode->i_private; @@ -183,11 +183,11 @@ static int guc_log_relay_release(struct inode *inode, struct file *file) return 0; } -static const struct file_operations guc_log_relay_fops = { +static const struct file_operations guc_log_relay_ctl_fops = { .owner = THIS_MODULE, - .open = guc_log_relay_open, - .write = guc_log_relay_write, - .release = guc_log_relay_release, + .open = guc_log_relay_ctl_open, + .write = guc_log_relay_ctl_write, + .release = guc_log_relay_ctl_release, }; void intel_guc_log_debugfs_register(struct intel_guc_log *log, @@ -197,7 +197,7 @@ void intel_guc_log_debugfs_register(struct intel_guc_log *log, { "guc_log_dump", &guc_log_dump_fops, NULL }, { "guc_load_err_log_dump", &guc_load_err_log_dump_fops, NULL }, { "guc_log_level", &guc_log_level_fops, NULL }, - { "guc_log_relay", &guc_log_relay_fops, NULL }, + { "guc_log_relay_ctl", &guc_log_relay_ctl_fops, NULL }, { "guc_log_relay_subbuf_size", &guc_log_relay_subbuf_size_fops, NULL }, { "guc_log_relay_subbuf_count", &guc_log_relay_subbuf_count_fops, NULL }, };
GuC log relay debugfs name for the control handle vs the actual relay channel are vague. Fix them so it's obvious from the name. Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 2 +- .../drm/i915/gt/uc/intel_guc_log_debugfs.c | 22 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-)