[PATCH] drm/i915: Add some more bits to CURSOR_POS_MASK

[PATCH] drm/i915: Add some more bits to CURSOR_POS_MASK

[Robert Fekete]

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(-)

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

Re: [PATCH] drm/i915: Add some more bits to CURSOR_POS_MASK

[Patrik Jakobsson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add some more bits to CURSOR_POS_MASK

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add some more bits to CURSOR_POS_MASK

[Robert Fekete]

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
> 

-- 
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

Re: [PATCH] drm/i915: Add some more bits to CURSOR_POS_MASK

[Patrik Jakobsson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx