Re: [PATCH v6 1/2] drm/xe: add a new debugfs file - mmio

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 06, 2024 at 05:16:23PM +0000, Koby Elbaz wrote:
> On 31/01/2024 22:14, Summers, Stuart wrote:
> > On Wed, 2024-01-31 at 21:59 +0200, Koby Elbaz wrote:
> >> Now that the former mmio handlers are fully removed, this
> >> debugfs file will be used for mmio access (read/write)
> >> through the debugfs ioctl file operation.
> > Sorry I don't know the history here of v0-v5, but one question, with
> > igt-gpu-tools/tools/intel_reg we can read and write MMIO registers
> > without needing to have the driver loaded at all (i.e. straight through
> > the BARs). Why do we want to also put this capability in the xe debugfs
> > then?
> >
> > Thanks,
> > Stuart
> 
> Hi Stuart,
> Sorry for the late response,
> To some extent we can use it, however, it does not satisfy our 
> development needs.
> Moreover, this change was recently agreed upon throughout the removal of 
> the former mmio-ioctl code:
> https://patchwork.freedesktop.org/series/123403/

If someone intends to hook this up into intel_reg then please
make sure it's _not_ the default path. When debugging I want my
register accesses to go directly to hardware and not be
potentially mucked up by a busted driver.

> 
> 
> Thanks,
> Koby Elbaz
> 
> >
> >> Signed-off-by: Koby Elbaz <kelbaz@habana.ai>
> >> ---
> >>   drivers/gpu/drm/xe/xe_debugfs.c | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c
> >> b/drivers/gpu/drm/xe/xe_debugfs.c
> >> index 01db5b27bec5..21b2257aa3a8 100644
> >> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> >> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> >> @@ -98,12 +98,22 @@ static int forcewake_release(struct inode *inode,
> >> struct file *file)
> >>          return 0;
> >>   }
> >>   
> >> +static long xe_debugfs_mmio_ioctl(struct file *f, unsigned int cmd,
> >> unsigned long arg)
> >> +{
> >> +       return 0;
> >> +}
> >> +
> >>   static const struct file_operations forcewake_all_fops = {
> >>          .owner = THIS_MODULE,
> >>          .open = forcewake_open,
> >>          .release = forcewake_release,
> >>   };
> >>   
> >> +static const struct file_operations mmio_fops = {
> >> +       .owner = THIS_MODULE,
> >> +       .unlocked_ioctl = xe_debugfs_mmio_ioctl,
> >> +};
> >> +
> >>   void xe_debugfs_register(struct xe_device *xe)
> >>   {
> >>          struct ttm_device *bdev = &xe->ttm;
> >> @@ -142,6 +152,8 @@ void xe_debugfs_register(struct xe_device *xe)
> >>          for_each_gt(gt, xe, id)
> >>                  xe_gt_debugfs_register(gt);
> >>   
> >> +       debugfs_create_file("mmio", 0644, root, xe, &mmio_fops);
> >> +
> >>   #ifdef CONFIG_FAULT_INJECTION
> >>          fault_create_debugfs_attr("fail_gt_reset", root,
> >> &gt_reset_failure);
> >>   #endif
> 
> 

-- 
Ville Syrjälä
Intel