| Message ID | 1446629719-1074-1-git-send-email-robert.fekete@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 04, 2015 at 10:35:19AM +0100, Robert Fekete wrote: > The old value of 0x7FF will wrap the position at 2048 giving wrong > coordinate values on panels larger than 2048 pixels in any direction. > Used in i915_debugfs atm. Looking at all hw specs available at 01.org > shows that X position is bit 0:11, and even 0:12 on some hw where > remaining bits up to bit 14 is MBZ. For Y position it is bits 16-27 > where bits 28:30 is MBZ. It should be safe to increase CURSOR_POS_MASK > to 13 bits (0x1FFF) making 8192 as a new wrap around value still getting > valid cursor positions on platforms with only 12bits available thanks to > MBZ on adjacent bits above. I cannot find documentation for older hardware and this only touches debugfs, so in worst case we get wrong values for really old hardware but good ones for newer. I think that's a fair tradeoff. Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > Signed-off-by: Robert Fekete <robert.fekete@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 894253228947..f351f46f8cb9 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4883,7 +4883,7 @@ enum skl_disp_power_wells { > #define CURSOR_TRICKLE_FEED_DISABLE (1 << 14) > #define _CURABASE 0x70084 > #define _CURAPOS 0x70088 > -#define CURSOR_POS_MASK 0x007FF > +#define CURSOR_POS_MASK 0x01FFF > #define CURSOR_POS_SIGN 0x8000 > #define CURSOR_X_SHIFT 0 > #define CURSOR_Y_SHIFT 16 > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 04, 2015 at 10:59:28AM +0100, Patrik Jakobsson wrote: > On Wed, Nov 04, 2015 at 10:35:19AM +0100, Robert Fekete wrote: > > The old value of 0x7FF will wrap the position at 2048 giving wrong > > coordinate values on panels larger than 2048 pixels in any direction. > > Used in i915_debugfs atm. Looking at all hw specs available at 01.org > > shows that X position is bit 0:11, and even 0:12 on some hw where > > remaining bits up to bit 14 is MBZ. For Y position it is bits 16-27 > > where bits 28:30 is MBZ. It should be safe to increase CURSOR_POS_MASK > > to 13 bits (0x1FFF) making 8192 as a new wrap around value still getting > > valid cursor positions on platforms with only 12bits available thanks to > > MBZ on adjacent bits above. > > I cannot find documentation for older hardware and this only touches > debugfs, so in worst case we get wrong values for really old hardware but good > ones for newer. I think that's a fair tradeoff. > > Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> If it's only used in debugfs then imo just drop it. Having a _MASK which isn't valid on all platforms, but where we don't have differnt #defines for the different platforms is really confusing. -Daniel > > > > > Signed-off-by: Robert Fekete <robert.fekete@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 894253228947..f351f46f8cb9 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -4883,7 +4883,7 @@ enum skl_disp_power_wells { > > #define CURSOR_TRICKLE_FEED_DISABLE (1 << 14) > > #define _CURABASE 0x70084 > > #define _CURAPOS 0x70088 > > -#define CURSOR_POS_MASK 0x007FF > > +#define CURSOR_POS_MASK 0x01FFF > > #define CURSOR_POS_SIGN 0x8000 > > #define CURSOR_X_SHIFT 0 > > #define CURSOR_Y_SHIFT 16 > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On ons, 2015-11-18 at 10:17 +0100, Daniel Vetter wrote: > On Wed, Nov 04, 2015 at 10:59:28AM +0100, Patrik Jakobsson wrote: > > On Wed, Nov 04, 2015 at 10:35:19AM +0100, Robert Fekete wrote: > > > The old value of 0x7FF will wrap the position at 2048 giving wrong > > > coordinate values on panels larger than 2048 pixels in any direction. > > > Used in i915_debugfs atm. Looking at all hw specs available at 01.org > > > shows that X position is bit 0:11, and even 0:12 on some hw where > > > remaining bits up to bit 14 is MBZ. For Y position it is bits 16-27 > > > where bits 28:30 is MBZ. It should be safe to increase CURSOR_POS_MASK > > > to 13 bits (0x1FFF) making 8192 as a new wrap around value still getting > > > valid cursor positions on platforms with only 12bits available thanks to > > > MBZ on adjacent bits above. > > > > I cannot find documentation for older hardware and this only touches > > debugfs, so in worst case we get wrong values for really old hardware but good > > ones for newer. I think that's a fair tradeoff. > > > > Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > If it's only used in debugfs then imo just drop it. Having a _MASK which > isn't valid on all platforms, but where we don't have differnt #defines > for the different platforms is really confusing. > -Daniel Well, not the most important patch around, but still the value of 0x7ff is still too conservative and gives wrong cursor pos values on large panels. I have hard times digging up really old register specs so I still can't see which Intel platform this new value of mine isn't valid on. It is this patch by Chris in debugfs that is broken on large panels wrapping coords at (x_pos/y_pos > 2048) http://marc.info/?l=git-commits-head&m=139697989108096&w=1 > > > > > > > > Signed-off-by: Robert Fekete <robert.fekete@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index 894253228947..f351f46f8cb9 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -4883,7 +4883,7 @@ enum skl_disp_power_wells { > > > #define CURSOR_TRICKLE_FEED_DISABLE (1 << 14) > > > #define _CURABASE 0x70084 > > > #define _CURAPOS 0x70088 > > > -#define CURSOR_POS_MASK 0x007FF > > > +#define CURSOR_POS_MASK 0x01FFF > > > #define CURSOR_POS_SIGN 0x8000 > > > #define CURSOR_X_SHIFT 0 > > > #define CURSOR_Y_SHIFT 16 > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Wed, Nov 25, 2015 at 1:54 PM, Robert Fekete <robert.fekete@linux.intel.com> wrote: > On ons, 2015-11-18 at 10:17 +0100, Daniel Vetter wrote: >> On Wed, Nov 04, 2015 at 10:59:28AM +0100, Patrik Jakobsson wrote: >> > On Wed, Nov 04, 2015 at 10:35:19AM +0100, Robert Fekete wrote: >> > > The old value of 0x7FF will wrap the position at 2048 giving wrong >> > > coordinate values on panels larger than 2048 pixels in any direction. >> > > Used in i915_debugfs atm. Looking at all hw specs available at 01.org >> > > shows that X position is bit 0:11, and even 0:12 on some hw where >> > > remaining bits up to bit 14 is MBZ. For Y position it is bits 16-27 >> > > where bits 28:30 is MBZ. It should be safe to increase CURSOR_POS_MASK >> > > to 13 bits (0x1FFF) making 8192 as a new wrap around value still getting >> > > valid cursor positions on platforms with only 12bits available thanks to >> > > MBZ on adjacent bits above. >> > >> > I cannot find documentation for older hardware and this only touches >> > debugfs, so in worst case we get wrong values for really old hardware but good >> > ones for newer. I think that's a fair tradeoff. >> > >> > Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> >> >> If it's only used in debugfs then imo just drop it. Having a _MASK which >> isn't valid on all platforms, but where we don't have differnt #defines >> for the different platforms is really confusing. >> -Daniel Yes, seems fair. > > Well, not the most important patch around, but still the value of 0x7ff > is still too conservative and gives wrong cursor pos values on large > panels. I have hard times digging up really old register specs so I > still can't see which Intel platform this new value of mine isn't valid > on. I think Daniel meant that we can remove the define altogether and hardcode a sane mask in debugfs instead. Keeping the define around might give people the wrong idea what the bspec actually says. Also adding a comment along with the hardcoded mask in debugfs would be nice. -Patrik > > It is this patch by Chris in debugfs that is broken on large panels > wrapping coords at (x_pos/y_pos > 2048) > http://marc.info/?l=git-commits-head&m=139697989108096&w=1 > >> > >> > > >> > > Signed-off-by: Robert Fekete <robert.fekete@linux.intel.com> >> > > --- >> > > drivers/gpu/drm/i915/i915_reg.h | 2 +- >> > > 1 file changed, 1 insertion(+), 1 deletion(-) >> > > >> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> > > index 894253228947..f351f46f8cb9 100644 >> > > --- a/drivers/gpu/drm/i915/i915_reg.h >> > > +++ b/drivers/gpu/drm/i915/i915_reg.h >> > > @@ -4883,7 +4883,7 @@ enum skl_disp_power_wells { >> > > #define CURSOR_TRICKLE_FEED_DISABLE (1 << 14) >> > > #define _CURABASE 0x70084 >> > > #define _CURAPOS 0x70088 >> > > -#define CURSOR_POS_MASK 0x007FF >> > > +#define CURSOR_POS_MASK 0x01FFF >> > > #define CURSOR_POS_SIGN 0x8000 >> > > #define CURSOR_X_SHIFT 0 >> > > #define CURSOR_Y_SHIFT 16 >> > > -- >> > > 1.9.1 >> > > >> > > _______________________________________________ >> > > Intel-gfx mailing list >> > > Intel-gfx@lists.freedesktop.org >> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > > -- > BR > /Robert Fekete > Intel Open Source Technology Center > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 894253228947..f351f46f8cb9 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4883,7 +4883,7 @@ enum skl_disp_power_wells { #define CURSOR_TRICKLE_FEED_DISABLE (1 << 14) #define _CURABASE 0x70084 #define _CURAPOS 0x70088 -#define CURSOR_POS_MASK 0x007FF +#define CURSOR_POS_MASK 0x01FFF #define CURSOR_POS_SIGN 0x8000 #define CURSOR_X_SHIFT 0 #define CURSOR_Y_SHIFT 16
The old value of 0x7FF will wrap the position at 2048 giving wrong coordinate values on panels larger than 2048 pixels in any direction. Used in i915_debugfs atm. Looking at all hw specs available at 01.org shows that X position is bit 0:11, and even 0:12 on some hw where remaining bits up to bit 14 is MBZ. For Y position it is bits 16-27 where bits 28:30 is MBZ. It should be safe to increase CURSOR_POS_MASK to 13 bits (0x1FFF) making 8192 as a new wrap around value still getting valid cursor positions on platforms with only 12bits available thanks to MBZ on adjacent bits above. Signed-off-by: Robert Fekete <robert.fekete@linux.intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)