Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.

Submitted by Michael Thayer on March 10, 2016, 7:17 a.m.

Details

Message ID 56E11F6F.3030506@oracle.com
State Superseded
Headers show

Not browsing as part of any series.

Patch hide | download patch | download mbox 

diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c 
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 0d34ca1..36c3093 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -485,44 +485,36 @@  drmmode_set_cursor_position(xf86CrtcPtr crtc, int 
x, int y)
      drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, 
x, y);
  }
  -static void
+static Bool
  drmmode_set_cursor(xf86CrtcPtr crtc)
  {
      drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
      drmmode_ptr drmmode = drmmode_crtc->drmmode;
      uint32_t handle = drmmode_crtc->cursor_bo->handle;
      modesettingPtr ms = modesettingPTR(crtc->scrn);
-    static Bool use_set_cursor2 = TRUE;
      int ret;
  -    if (use_set_cursor2) {
-        xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
-        CursorPtr cursor = xf86_config->cursor;
-
-        ret =
-            drmModeSetCursor2(drmmode->fd, 
drmmode_crtc->mode_crtc->crtc_id,
-                              handle, ms->cursor_width, ms->cursor_height,
-                              cursor->bits->xhot, cursor->bits->yhot);
-        if (!ret)
-            return;
-        if (ret == -EINVAL)
-            use_set_cursor2 = FALSE;
-    }
+    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
+    CursorPtr cursor = xf86_config->cursor;
  -    ret = drmModeSetCursor(drmmode->fd, 
drmmode_crtc->mode_crtc->crtc_id, handle,
-                           ms->cursor_width, ms->cursor_height);
+    ret =
+        drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
+                          handle, ms->cursor_width, ms->cursor_height,
+                          cursor->bits->xhot, cursor->bits->yhot);
+    if (!ret)
+        return TRUE;
  -    if (ret) {
-        xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
-        xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
+    /* -EINVAL can mean bad cursor parameters or Cursor2 API not 
supported. */
+    if (ret == -EINVAL)
+        ret = drmModeSetCursor(drmmode->fd, 
drmmode_crtc->mode_crtc->crtc_id,
+                               handle, ms->cursor_width, 
ms->cursor_height);
  -        cursor_info->MaxWidth = cursor_info->MaxHeight = 0;
-        drmmode_crtc->drmmode->sw_cursor = TRUE;
-        /* fallback to swcursor */
-    }
+    if (ret)
+        return FALSE; /* fallback to swcursor */
+    return TRUE;
  }
  -static void
+static Bool
  drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image)
  {

Comments

Olivier Fourdan March 10, 2016, 8:11 a.m. 
Hi Michael,

[...]
> Re-sending as this did not seem to get noticed much the first time.

I am not doing a formal review of your patch as it's outside of my area of competences, just a few comments, trying to help.

I am not sure re-sending the patch as-is is the best course of action here, usually simply replying the original message with a gentle reminder suffices.

Reason being that the original patch (sent only 4 days ago) is still accessible in patchwork and still marked as "New":

https://patchwork.freedesktop.org/patch/75985/

And the new here is also available:

https://patchwork.freedesktop.org/patch/76437/

So you should mark on of the two as superseded in patchwork. But, yes, patches do take some time for review.

Another problem is this patch here won't apply here due to formatting issues apparently:

  $ pwc git-am 76437
  Applying patch #76437 using 'git am'
  Description: Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.
  Applying: Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.
  fatal: corrupt patch at line 8
  Patch failed at 0001 Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.

See below. But the original patch (the first one) doesn't seem to suffer from the same formatting issues though.

>   hw/xfree86/drivers/modesetting/drmmode_display.c | 47
> ++++++++++--------------
>   1 file changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 0d34ca1..36c3093 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int
> x, int y)
>       drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
> x, y);
>   }
>   -static void
> +static Bool
>   drmmode_set_cursor(xf86CrtcPtr crtc)
>   {
>       drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>       drmmode_ptr drmmode = drmmode_crtc->drmmode;
>       uint32_t handle = drmmode_crtc->cursor_bo->handle;
>       modesettingPtr ms = modesettingPTR(crtc->scrn);
> -    static Bool use_set_cursor2 = TRUE;
>       int ret;
>   -    if (use_set_cursor2) {

That won't apply. There are several occurrences of the same issue in the patch being resent, I think best would be to use git-send-email to send patches instead of Thunderbird -as seen in the message source- (or any other MUA for that matter) that can break the patch formatting and confuse git.

HTH,
Cheers,
Olivier
Michael Thayer March 10, 2016, 8:25 a.m. 
Hello Olivier,

On 10.03.2016 09:11, Olivier Fourdan wrote:
> Hi Michael,
>
> [...]
>> Re-sending as this did not seem to get noticed much the first
>> time.
>
> I am not doing a formal review of your patch as it's outside of my
> area of competences, just a few comments, trying to help.

Thank you, help welcome.

> I am not sure re-sending the patch as-is is the best course of action
> here, usually simply replying the original message with a gentle
> reminder suffices.
>
> Reason being that the original patch (sent only 4 days ago) is still
> accessible in patchwork and still marked as "New":
>
> https://patchwork.freedesktop.org/patch/75985/
>
> And the new here is also available:
>
> https://patchwork.freedesktop.org/patch/76437/
>
> So you should mark on of the two as superseded in patchwork. But,
> yes, patches do take some time for review.

Right, I am still not too familiar with the patch process for X.Org. 
How do I mark the re-sent one, which as you said does not apply cleanly, 
as superseded?

> Another problem is this patch here won't apply here due to formatting
> issues apparently:
>
> $ pwc git-am 76437 Applying patch #76437 using 'git am' Description:
> Re-send: [PATCH] modesetting: allow switching from software to
> hardware cursors. Applying: Re-send: [PATCH] modesetting: allow
> switching from software to hardware cursors. fatal: corrupt patch at
> line 8 Patch failed at 0001 Re-send: [PATCH] modesetting: allow
> switching from software to hardware cursors.
>
> See below. But the original patch (the first one) doesn't seem to
> suffer from the same formatting issues though.
>
>> hw/xfree86/drivers/modesetting/drmmode_display.c | 47
>> ++++++++++-------------- 1 file changed, 20 insertions(+), 27
>> deletions(-)
>>
>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
>> b/hw/xfree86/drivers/modesetting/drmmode_display.c index
>> 0d34ca1..36c3093 100644 ---
>> a/hw/xfree86/drivers/modesetting/drmmode_display.c +++
>> b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -485,44
>> +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int
>> y) drmModeMoveCursor(drmmode->fd,
>> drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static
>> Bool drmmode_set_cursor(xf86CrtcPtr crtc) {
>> drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>> drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle =
>> drmmode_crtc->cursor_bo->handle; modesettingPtr ms =
>> modesettingPTR(crtc->scrn); -    static Bool use_set_cursor2 =
>> TRUE; int ret; -    if (use_set_cursor2) {
>
> That won't apply. There are several occurrences of the same issue in
> the patch being resent, I think best would be to use git-send-email
> to send patches instead of Thunderbird -as seen in the message
> source- (or any other MUA for that matter) that can break the patch
> formatting and confuse git.

Indeed, I sent the original with g-s-e and the re-send with Thunderbird.

Thanks again.

Regards,

Michael

> HTH, Cheers, Olivier
Olivier Fourdan March 10, 2016, 8:44 a.m. 
Hi,

----- Original Message -----
> > [...]
> Right, I am still not too familiar with the patch process for X.Org.
> How do I mark the re-sent one, which as you said does not apply cleanly,
> as superseded?

You need to create an account in patchwork https://patchwork.freedesktop.org/ and link that account with your email address that you used to submit the patches, so you can change the status of those patches you submitted.

You may also install pwclient and the relevant .pwclientrc so you can use patchwork from the command line as well, very handy.

See Damien Lespiau's blog post here: http://damien.lespiau.name/2016/02/augmenting-mailing-lists-with-patchwork.html

Cheers,
Olivier