Re: [PATCH] drm/xe: Fix build error in xe_ggtt.c

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Feb 28, 2024 at 12:49:05PM -0600, Lucas De Marchi wrote:
> On Wed, Feb 28, 2024 at 03:47:13PM +0100, Thomas Hellström wrote:
> > On Mon, 2024-02-26 at 15:40 +0000, Matthew Brost wrote:
> > > On Mon, Feb 26, 2024 at 10:05:58AM +0100, Thomas Hellström wrote:
> > > > On Sat, 2024-02-24 at 16:14 -0800, Matthew Brost wrote:
> > > > > Need to include io-64-nonatomic-lo-hi.h for writeq function.
> > > >
> > > > As I understand it, the choice of header here determines the dword
> > > > write order on 32-bit systems that don't have an atomic writeq(),
> > > >
> > > > So is writing the low dword first the correct order in this case?
> > > > Perhaps add a motivation in the commit message?
> > > >
> > > 
> > > "Cleanup some layering in GGTT" removed xe_mmio.h from xe_gt.c and
> > > that
> > > file includes linux/io-64-nonatomic-lo-hi.h. Perhaps it is better
> > > just
> > > to include xe_mmio.h again in xe_gt.c?
> > 
> > I think it then makes sense to use your original patch, so that it's
> > easier to follow for each subsystem what ordering is used.
> 
> what original patch, I only find this one in the mailing list.
> and it seems correct to me, following the "include what you use"
> approach.
> 

I'm also confused by Thomas comment.

Let me ask directly - do you prefer xe_mmio.h or
linux/io-64-nonatomic-lo-hi.h to be included in xe_ggtt.c?

Matt

> Also see some more details in commit 9a6e6c14bfde ("drm/xe/mmio: Use
> non-atomic writeq/readq variant for 32b"):
> 
> 	writeq() and readq() and other functions working on 64 bit variables
> 	are not provided by 32b arch. For that it's needed to choose between
> 	linux/io-64-nonatomic-hi-lo.h and linux/io-64-nonatomic-lo-hi.h,
> 	spliting the read/write in 2 accesses. For xe driver, it doesn't matter
> 	much, so just choose one and include in xe_mmio.h.
> 
> Aside from the "Fixes:" trailer, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> thanks
> Lucas De Marchi
> 
> > 
> > Also note Jani's comment. We should use "dim fixes <hash>" to get the
> > fixes tag correct.
> > 
> > Thanks,
> > Thomas
> > 
> > 
> > > 
> > > Matt
> > > 
> > > > /Thomas
> > > >
> > > >
> > > > >
> > > > > Fixes: 3121fed0c51b drm/xe: ("Cleanup some layering in GGTT")
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > Closes:
> > > > > https://lore.kernel.org/oe-kbuild-all/202402241903.R5J8hKVI-lkp@intel.com/
> > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/xe/xe_ggtt.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/xe/xe_ggtt.c
> > > > > b/drivers/gpu/drm/xe/xe_ggtt.c
> > > > > index 5d46958e3144..717d0e76277a 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_ggtt.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> > > > > @@ -5,6 +5,7 @@
> > > > >  
> > > > >  #include "xe_ggtt.h"
> > > > >  
> > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > >  #include <linux/sizes.h>
> > > > >  
> > > > >  #include <drm/drm_managed.h>
> > > >
> >