All of lore.kernel.org
 help / color / mirror / Atom feed
From: alexdeucher@gmail.com (Alex Deucher)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 3/5] DRM: Armada: Add support for ARGB 32x64 or 64x32 hardware cursors
Date: Fri, 18 Oct 2013 10:31:57 -0400	[thread overview]
Message-ID: <CADnq5_Nc9ZK94BO=S7wK=nVoKVUFkJowxo2MvBNMSj+6-cPHUw@mail.gmail.com> (raw)
In-Reply-To: <CAF6AEGt1uOsE5V6rAtxpLNNaYkaKiw9G8CNdt31JZVdS6TX-hA@mail.gmail.com>

On Thu, Oct 17, 2013 at 7:58 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, Oct 7, 2013 at 8:50 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Mon, Oct 07, 2013 at 03:29:15PM +0300, Siarhei Siamashka wrote:
>>> On Mon, 7 Oct 2013 11:32:50 +0100
>>> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>>> > However, what you're suggesting is dangerous.  What do you do when you're
>>> > presented with a cursor which is 64x64 ?  Do you:
>>> >
>>> > (a) not display it
>>> > (b) crash the X server
>>> >
>>> > There isn't a fallback to using software cursors available in the X server
>>> > because there's no error reporting for a failed hardware cursor update.
>>>
>>> Hmm, maybe I'm missing something, but doesn't returning FALSE from
>>> UseHWCursorARGB just make the X server fallback to using a software
>>> cursor?
>>>
>>> https://github.com/ssvb/xf86-video-fbturbo/blob/689c08256555/src/sunxi_disp_hwcursor.c#L72
>>>
>>> I'm just doing this. And also have hooks to notify the DRI2 code about
>>> the status of the cursor. In the case if the hardware cursor is not
>>> enabled, hardware overlays can't be safely used with the software
>>> cursor and we need to fallback to CPU/GPU copy instead of zero-copy
>>> buffers swapping.
>>>
>>> And I fully agree that it is the responsibility of the kernel to expose
>>> as much of the hardware capabilities as possible (without compromising
>>> reliability and/or security).
>>
>> If you wish to go in below the level presented by
>> hw/xfree86/modes/xf86Cursors.c (as you are doing), then yes you can do
>> this, and it's something which maybe should be looked into.  You have a
>> good reason to do this (due to your hardware cursor being incompatible
>> with overlays).
>>
>> We have no such restriction - the only issue here is the "odd" size of
>> cursors.  Practically, choosing one or other of 64x32 or 32x64 works
>> fine for the cases I've seen.  I haven't seen any cursors which are
>> 64x64 yet.  I'm not saying they don't exist though!  I can imagine that
>> some accessibility options might want to display a large cursor.
>>
>> Now, the way I handle this is that the kernel allows _either_ a
>> (1 to 32)x(1 to 64) or (1 to 64)x(1 to 32) cursor.  Both width and
>> height larger than 32 gets rejected with -EINVAL:
>>
>> +               /* maximum size is 64x32 or 32x64 */
>> +               if (w > 64 || h > 64 || (w > 32 && h > 32))
>> +                       return -ENOMEM;
>>
>> which is quite reasonable for this hardware.  However, there is no way
>> to export this from DRM - adding maximum cursor size properties wouldn't
>> really represent this.
>
> Until relatively recently, it had always been a device specific ddx
> which would somehow know what w/h values to pass to
> xf86_cursors_init().  I think for xf86-video-modesetting and wayland
> compositors, we probably need to come up with something better.
> Although off the top of my head I can't think of a good way to express
> 64x32 or 32x64.. that is a weird restriction.
>

We have a similar problem on our newer asics.  They require a 128x128
cursors so when you use xf86-video-modesetting, you end up with messed
up cursors because it assumes 64x64.  We could add cursor size to drm
caps.

Alex

> Anyways, right now I don't think there is anything different that I'd
> do in the kernel part.  I *suppose* you could try and figure out that
> all the alpha values are either 0x00 or 0xff and support 64x64 in that
> special case.  I'm not sure that is actually better.. it at least
> makes the failure vs success cases more confusing.  And unless you are
> rockin' the twm+xterm+xcalc setup, I doubt you will see many cursors
> with only 0x00 or 0xff alpha.
>
> So,
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
>
>> The only question is whether DRM should export some capabilities to
>> indicate what's possible with the cursor, so that X knows about this
>> quirk of this hardware.  Overall, I suspect that it's just rare and
>> not worth the effort.  I think it's just easier to pick one of these
>> and stick with it, and if we happen to encounter a cursor larger than
>> our chosen dimentions, XFree86 will already automatically fall back
>> to s/w cursor.
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: Alex Deucher <alexdeucher@gmail.com>
To: Rob Clark <robdclark@gmail.com>
Cc: "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Jason Cooper <jason@lakedaemon.net>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Subject: Re: [PATCH 3/5] DRM: Armada: Add support for ARGB 32x64 or 64x32 hardware cursors
Date: Fri, 18 Oct 2013 10:31:57 -0400	[thread overview]
Message-ID: <CADnq5_Nc9ZK94BO=S7wK=nVoKVUFkJowxo2MvBNMSj+6-cPHUw@mail.gmail.com> (raw)
In-Reply-To: <CAF6AEGt1uOsE5V6rAtxpLNNaYkaKiw9G8CNdt31JZVdS6TX-hA@mail.gmail.com>

On Thu, Oct 17, 2013 at 7:58 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, Oct 7, 2013 at 8:50 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Mon, Oct 07, 2013 at 03:29:15PM +0300, Siarhei Siamashka wrote:
>>> On Mon, 7 Oct 2013 11:32:50 +0100
>>> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>>> > However, what you're suggesting is dangerous.  What do you do when you're
>>> > presented with a cursor which is 64x64 ?  Do you:
>>> >
>>> > (a) not display it
>>> > (b) crash the X server
>>> >
>>> > There isn't a fallback to using software cursors available in the X server
>>> > because there's no error reporting for a failed hardware cursor update.
>>>
>>> Hmm, maybe I'm missing something, but doesn't returning FALSE from
>>> UseHWCursorARGB just make the X server fallback to using a software
>>> cursor?
>>>
>>> https://github.com/ssvb/xf86-video-fbturbo/blob/689c08256555/src/sunxi_disp_hwcursor.c#L72
>>>
>>> I'm just doing this. And also have hooks to notify the DRI2 code about
>>> the status of the cursor. In the case if the hardware cursor is not
>>> enabled, hardware overlays can't be safely used with the software
>>> cursor and we need to fallback to CPU/GPU copy instead of zero-copy
>>> buffers swapping.
>>>
>>> And I fully agree that it is the responsibility of the kernel to expose
>>> as much of the hardware capabilities as possible (without compromising
>>> reliability and/or security).
>>
>> If you wish to go in below the level presented by
>> hw/xfree86/modes/xf86Cursors.c (as you are doing), then yes you can do
>> this, and it's something which maybe should be looked into.  You have a
>> good reason to do this (due to your hardware cursor being incompatible
>> with overlays).
>>
>> We have no such restriction - the only issue here is the "odd" size of
>> cursors.  Practically, choosing one or other of 64x32 or 32x64 works
>> fine for the cases I've seen.  I haven't seen any cursors which are
>> 64x64 yet.  I'm not saying they don't exist though!  I can imagine that
>> some accessibility options might want to display a large cursor.
>>
>> Now, the way I handle this is that the kernel allows _either_ a
>> (1 to 32)x(1 to 64) or (1 to 64)x(1 to 32) cursor.  Both width and
>> height larger than 32 gets rejected with -EINVAL:
>>
>> +               /* maximum size is 64x32 or 32x64 */
>> +               if (w > 64 || h > 64 || (w > 32 && h > 32))
>> +                       return -ENOMEM;
>>
>> which is quite reasonable for this hardware.  However, there is no way
>> to export this from DRM - adding maximum cursor size properties wouldn't
>> really represent this.
>
> Until relatively recently, it had always been a device specific ddx
> which would somehow know what w/h values to pass to
> xf86_cursors_init().  I think for xf86-video-modesetting and wayland
> compositors, we probably need to come up with something better.
> Although off the top of my head I can't think of a good way to express
> 64x32 or 32x64.. that is a weird restriction.
>

We have a similar problem on our newer asics.  They require a 128x128
cursors so when you use xf86-video-modesetting, you end up with messed
up cursors because it assumes 64x64.  We could add cursor size to drm
caps.

Alex

> Anyways, right now I don't think there is anything different that I'd
> do in the kernel part.  I *suppose* you could try and figure out that
> all the alpha values are either 0x00 or 0xff and support 64x64 in that
> special case.  I'm not sure that is actually better.. it at least
> makes the failure vs success cases more confusing.  And unless you are
> rockin' the twm+xterm+xcalc setup, I doubt you will see many cursors
> with only 0x00 or 0xff alpha.
>
> So,
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
>
>> The only question is whether DRM should export some capabilities to
>> indicate what's possible with the cursor, so that X knows about this
>> quirk of this hardware.  Overall, I suspect that it's just rare and
>> not worth the effort.  I think it's just easier to pick one of these
>> and stick with it, and if we happen to encounter a cursor larger than
>> our chosen dimentions, XFree86 will already automatically fall back
>> to s/w cursor.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2013-10-18 14:31 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-06 22:07 [PATCH 0/5] Armada DRM stuff Russell King - ARM Linux
2013-10-06 22:07 ` Russell King - ARM Linux
2013-10-06 22:07 ` [PATCH 1/5] drm/i2c: tda998x: set VIF for full range, underscanned display Russell King
2013-10-06 22:07   ` Russell King
2013-10-07  8:59   ` Jean-Francois Moine
2013-10-07  8:59     ` Jean-Francois Moine
2013-10-18 15:00     ` Rob Clark
2013-10-18 15:00       ` Rob Clark
2013-10-06 22:08 ` [PATCH 2/5] DRM: Armada: Add Armada DRM driver Russell King
2013-10-06 22:08   ` Russell King
2013-10-10 21:25   ` Rob Clark
2013-10-10 21:25     ` Rob Clark
2013-10-10 21:59     ` Russell King - ARM Linux
2013-10-10 21:59       ` Russell King - ARM Linux
2013-10-10 22:23       ` Rob Clark
2013-10-10 22:23         ` Rob Clark
2013-10-10 22:53         ` Russell King - ARM Linux
2013-10-10 22:53           ` Russell King - ARM Linux
2013-10-11  0:10           ` Rob Clark
2013-10-11  0:10             ` Rob Clark
2013-10-06 22:09 ` [PATCH 3/5] DRM: Armada: Add support for ARGB 32x64 or 64x32 hardware cursors Russell King
2013-10-06 22:09   ` Russell King
2013-10-07  9:01   ` Jean-Francois Moine
2013-10-07  9:01     ` Jean-Francois Moine
2013-10-07  9:40     ` Russell King - ARM Linux
2013-10-07  9:40       ` Russell King - ARM Linux
2013-10-07 10:09       ` Jean-Francois Moine
2013-10-07 10:09         ` Jean-Francois Moine
2013-10-07 10:32         ` Russell King - ARM Linux
2013-10-07 10:32           ` Russell King - ARM Linux
2013-10-07 12:29           ` Siarhei Siamashka
2013-10-07 12:29             ` Siarhei Siamashka
2013-10-07 12:50             ` Russell King - ARM Linux
2013-10-07 12:50               ` Russell King - ARM Linux
2013-10-17 23:58               ` Rob Clark
2013-10-17 23:58                 ` Rob Clark
2013-10-18 14:31                 ` Alex Deucher [this message]
2013-10-18 14:31                   ` Alex Deucher
2013-10-06 22:10 ` [PATCH 4/5] DRM: Armada: start of MMP2/MMP3 support Russell King
2013-10-06 22:10   ` Russell King
2013-10-18  0:11   ` Rob Clark
2013-10-18  0:11     ` Rob Clark
2013-10-06 22:11 ` [PATCH 5/5] DRM: Armada: add support for drm tda19988 driver Russell King
2013-10-06 22:11   ` Russell King
2013-10-07  9:18   ` Jean-Francois Moine
2013-10-07  9:18     ` Jean-Francois Moine
2013-10-07  9:44     ` Russell King - ARM Linux
2013-10-07  9:44       ` Russell King - ARM Linux
2013-10-07 10:48       ` Jean-Francois Moine
2013-10-07 10:48         ` Jean-Francois Moine
2013-10-07 11:09         ` Russell King - ARM Linux
2013-10-07 11:09           ` Russell King - ARM Linux
2013-10-07 11:29           ` Sebastian Hesselbarth
2013-10-07 11:29             ` Sebastian Hesselbarth
2013-10-07 15:53             ` Mark Brown
2013-10-07 15:53               ` Mark Brown
2013-10-07 16:08               ` Sebastian Hesselbarth
2013-10-07 16:08                 ` Sebastian Hesselbarth
2013-10-07 17:05                 ` Mark Brown
2013-10-07 17:05                   ` Mark Brown
2013-10-07 12:03           ` Jean-Francois Moine
2013-10-07 12:03             ` Jean-Francois Moine
2013-10-07 12:36             ` Russell King - ARM Linux
2013-10-07 12:36               ` Russell King - ARM Linux
2013-10-07 14:59           ` Rob Clark
2013-10-07 14:59             ` Rob Clark
2013-10-08  9:19             ` Jean-Francois Moine
2013-10-08  9:19               ` Jean-Francois Moine
2013-10-08  9:49               ` Russell King - ARM Linux
2013-10-08  9:49                 ` Russell King - ARM Linux
2013-10-08 15:34                 ` Jean-Francois Moine
2013-10-08 15:34                   ` Jean-Francois Moine
2013-10-18  0:20                   ` Rob Clark
2013-10-18  0:20                     ` Rob Clark
2013-10-08 12:07               ` Rob Clark
2013-10-08 12:07                 ` Rob Clark
2013-10-07 21:47 ` [PATCH 0/5] Armada DRM stuff Sebastian Hesselbarth
2013-10-07 21:47   ` Sebastian Hesselbarth
2013-10-09 14:31   ` Russell King - ARM Linux
2013-10-09 14:31     ` Russell King - ARM Linux
2013-10-09 14:48     ` Rob Clark
2013-10-09 14:48       ` Rob Clark
2013-10-18 15:15 ` [GIT PULL] Armada DRM support Russell King - ARM Linux
2013-10-18 15:15   ` Russell King - ARM Linux
2013-10-22 13:36   ` Russell King - ARM Linux
2013-10-22 13:36     ` Russell King - ARM Linux

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CADnq5_Nc9ZK94BO=S7wK=nVoKVUFkJowxo2MvBNMSj+6-cPHUw@mail.gmail.com' \
    --to=alexdeucher@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.