| Message ID | 20240408170426.9285-13-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Increase COMPILE_TEST=y coverage | expand |
On 08/04/2024 20:04, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > __iowmb() isn't available on most architectures. Make > its use optional so that the driver can be built on > other architectures with COMPILE_TEST=y. > > Cc: Jyri Sarha <jyri.sarha@iki.fi> > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h > index f90e2dc3457c..44e4ada30fba 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h > +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h > @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) > #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) > iowrite64(data, addr); > #else > +#ifdef __iowmb > __iowmb(); > +#endif > /* This compiles to strd (=64-bit write) on ARM7 */ > *(volatile u64 __force *)addr = __cpu_to_le64(data); > #endif As the memory barrier is an important part there, would it be better to ifdef based on COMPILE_TEST, to make it clear why it's being done? Tomi
On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: > On 08/04/2024 20:04, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > __iowmb() isn't available on most architectures. Make > > its use optional so that the driver can be built on > > other architectures with COMPILE_TEST=y. > > > > Cc: Jyri Sarha <jyri.sarha@iki.fi> > > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h > > index f90e2dc3457c..44e4ada30fba 100644 > > --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h > > @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) > > #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) > > iowrite64(data, addr); > > #else > > +#ifdef __iowmb > > __iowmb(); > > +#endif > > /* This compiles to strd (=64-bit write) on ARM7 */ > > *(volatile u64 __force *)addr = __cpu_to_le64(data); > > #endif > > As the memory barrier is an important part there, would it be better to > ifdef based on COMPILE_TEST, to make it clear why it's being done? I can do that if you prefer. I suppose the real question is why iowrite64() doesn't work if a hand rolled version does work?
On 10/04/2024 18:25, Ville Syrjälä wrote: > On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: >> On 08/04/2024 20:04, Ville Syrjala wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> __iowmb() isn't available on most architectures. Make >>> its use optional so that the driver can be built on >>> other architectures with COMPILE_TEST=y. >>> >>> Cc: Jyri Sarha <jyri.sarha@iki.fi> >>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> --- >>> drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h >>> index f90e2dc3457c..44e4ada30fba 100644 >>> --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h >>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h >>> @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) >>> #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) >>> iowrite64(data, addr); >>> #else >>> +#ifdef __iowmb >>> __iowmb(); >>> +#endif >>> /* This compiles to strd (=64-bit write) on ARM7 */ >>> *(volatile u64 __force *)addr = __cpu_to_le64(data); >>> #endif >> >> As the memory barrier is an important part there, would it be better to >> ifdef based on COMPILE_TEST, to make it clear why it's being done? > > I can do that if you prefer. > > I suppose the real question is why iowrite64() doesn't work > if a hand rolled version does work? If I recall right, there is (was?) no iowrite64. The bus is 32 bit anyway. But the two 32 bit registers written with the tilcdc_write64() have an annoying HW race, so we tried to find a method of writing them that reduces the chance of race to a minimum. Tomi
On Wed, Apr 10, 2024 at 06:25:17PM +0300, Ville Syrjälä wrote: > On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: > > On 08/04/2024 20:04, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > __iowmb() isn't available on most architectures. Make > > > its use optional so that the driver can be built on > > > other architectures with COMPILE_TEST=y. > > > > > > Cc: Jyri Sarha <jyri.sarha@iki.fi> > > > Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h > > > index f90e2dc3457c..44e4ada30fba 100644 > > > --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h > > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h > > > @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) > > > #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) > > > iowrite64(data, addr); > > > #else > > > +#ifdef __iowmb > > > __iowmb(); > > > +#endif > > > /* This compiles to strd (=64-bit write) on ARM7 */ > > > *(volatile u64 __force *)addr = __cpu_to_le64(data); > > > #endif > > > > As the memory barrier is an important part there, would it be better to > > ifdef based on COMPILE_TEST, to make it clear why it's being done? > > I can do that if you prefer. What if someone tries to actually boot a kernel built with COMPILE_TEST=y on a machine with this hardware?
On 10/04/2024 20:04, Ville Syrjälä wrote: > On Wed, Apr 10, 2024 at 06:25:17PM +0300, Ville Syrjälä wrote: >> On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: >>> On 08/04/2024 20:04, Ville Syrjala wrote: >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> >>>> __iowmb() isn't available on most architectures. Make >>>> its use optional so that the driver can be built on >>>> other architectures with COMPILE_TEST=y. >>>> >>>> Cc: Jyri Sarha <jyri.sarha@iki.fi> >>>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h >>>> index f90e2dc3457c..44e4ada30fba 100644 >>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h >>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h >>>> @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) >>>> #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) >>>> iowrite64(data, addr); >>>> #else >>>> +#ifdef __iowmb >>>> __iowmb(); >>>> +#endif >>>> /* This compiles to strd (=64-bit write) on ARM7 */ >>>> *(volatile u64 __force *)addr = __cpu_to_le64(data); >>>> #endif >>> >>> As the memory barrier is an important part there, would it be better to >>> ifdef based on COMPILE_TEST, to make it clear why it's being done? >> >> I can do that if you prefer. > > What if someone tries to actually boot a kernel built > with COMPILE_TEST=y on a machine with this hardware? Ah, right... #ifndef __iowmb #define __iowmb BUG #endif ? =) Maybe go with the original one, but with a comment like "allow compilation without __iowmb() for COMPILE_TEST" or such. Tomi
April 10, 2024 at 8:04 PM, "Ville Syrjälä" <ville.syrjala@linux.intel.com mailto:ville.syrjala@linux.intel.com?to=%22Ville%20Syrj%C3%A4l%C3%A4%22%20%3Cville.syrjala%40linux.intel.com%3E > wrote: > > What if someone tries to actually boot a kernel built > with COMPILE_TEST=y on a machine with this hardware? > I doubt there is am335x HW out there with enough memory to load COMPILE_TEST=y kernel. BR, Jyri
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f90e2dc3457c..44e4ada30fba 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) iowrite64(data, addr); #else +#ifdef __iowmb __iowmb(); +#endif /* This compiles to strd (=64-bit write) on ARM7 */ *(volatile u64 __force *)addr = __cpu_to_le64(data); #endif