drm/msm/display: negative x/y in cursor move

modesetting X11 driver may provide negative x/y cordinates in mdp5_crtc_cursor_move call when rotation is enabled.

Cursor buffer can overlap down to its negative width/height.

ROI has to be recalculated for negative x/y indicating using the lower/right corner of the cursor buffer and hotspot must be set in MDP5_LM_CURSOR_XY_SRC_Y MDP5_LM_CURSOR_XY_SRC_X.

Signed-off-by Carsten Behling <[email protected]>

---
Changes in v2:
- fixed format specifier in debug message

  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 51 ++++++++++++++++++++++++++-----
  1 file changed, 43 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +40 -6

index 10271359789e..a7f4a6688fec 100644
@@ -65,7 +65,7 @@ struct mdp5_crtc {
  		struct drm_gem_object *scanout_bo;
  		uint64_t iova;
  		uint32_t width, height;
-		uint32_t x, y;
+		int x, y;
  	} cursor;
  };
  #define to_mdp5_crtc(x) container_of(x, struct mdp5_crtc, base)

@@ -760,20 +760,31 @@ static void get_roi(struct drm_crtc *crtc, uint32_t *roi_w, uint32_t *roi_h)
  	 * Cursor Region Of Interest (ROI) is a plane read from cursor
  	 * buffer to render. The ROI region is determined by the visibility of
  	 * the cursor point. In the default Cursor image the cursor point will
-	 * be at the top left of the cursor image, unless it is specified
-	 * otherwise using hotspot feature.
+	 * be at the top left of the cursor image.
  	 *
+	 * Without rotation:
  	 * If the cursor point reaches the right (xres - x < cursor.width) or
  	 * bottom (yres - y < cursor.height) boundary of the screen, then ROI
  	 * width and ROI height need to be evaluated to crop the cursor image
  	 * accordingly.
  	 * (xres-x) will be new cursor width when x > (xres - cursor.width)
  	 * (yres-y) will be new cursor height when y > (yres - cursor.height)
+	 *
+	 * With rotation:
+	 * We get negative x and/or y coordinates.
+	 * (cursor.width - abs(x)) will be new cursor width when x < 0
+	 * (cursor.height - abs(y)) will be new cursor width when y < 0
  	 */
-	*roi_w = min(mdp5_crtc->cursor.width, xres -
+	if (mdp5_crtc->cursor.x >= 0)
+		*roi_w = min(mdp5_crtc->cursor.width, xres -
  			mdp5_crtc->cursor.x);
-	*roi_h = min(mdp5_crtc->cursor.height, yres -
+	else
+		*roi_w = mdp5_crtc->cursor.width - abs(mdp5_crtc->cursor.x);
+	if (mdp5_crtc->cursor.y >= 0)
+		*roi_h = min(mdp5_crtc->cursor.height, yres -
  			mdp5_crtc->cursor.y);
+	else
+		*roi_h = mdp5_crtc->cursor.height - abs(mdp5_crtc->cursor.y);
  }
 
  static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)

@@ -783,7 +794,7 @@ static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
  	struct mdp5_kms *mdp5_kms = get_kms(crtc);
  	const enum mdp5_cursor_alpha cur_alpha = CURSOR_ALPHA_PER_PIXEL;
  	uint32_t blendcfg, stride;
-	uint32_t x, y, width, height;
+	uint32_t x, y, src_x, src_y, width, height;
  	uint32_t roi_w, roi_h;
  	int lm;
 

@@ -800,6 +811,26 @@ static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
 
  	get_roi(crtc, &roi_w, &roi_h);
 
+	/* If cusror buffer overlaps due to rotation on the
+	 * upper or left screen border the pixel offset inside
+	 * the cursor buffer of the ROI is the positive overlap
+	 * distance.
+	 */
+	if (mdp5_crtc->cursor.x < 0) {
+		src_x = abs(mdp5_crtc->cursor.x);
+		x = 0;
+	} else {
+		src_x = 0;
+	}
+	if (mdp5_crtc->cursor.y < 0) {
+		src_y = abs(mdp5_crtc->cursor.y);
+		y = 0;
+	} else {
+		src_y = 0;
+	}
+	DBG("%s: x=%u, y=%u roi_w=%u roi_h=%u src_x=%u src_y=%u",
+		crtc->name, x, y, roi_w, roi_h, src_x, src_y);
+
  	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm), stride);
  	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
  			MDP5_LM_CURSOR_FORMAT_FORMAT(CURSOR_FMT_ARGB8888));

@@ -812,6 +843,9 @@ static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
  	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
  			MDP5_LM_CURSOR_START_XY_Y_START(y) |
  			MDP5_LM_CURSOR_START_XY_X_START(x));
+	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_XY(lm),
+			MDP5_LM_CURSOR_XY_SRC_Y(src_y) |
+			MDP5_LM_CURSOR_XY_SRC_X(src_x));
  	mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm),
  			mdp5_crtc->cursor.iova);
 

@@ -932,8 +966,9 @@ static int mdp5_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
  	if (unlikely(!crtc->state->enable))

Thanks for the patch. Could you tell how to reproduce this issue on a db410c?

I was playing with xrandr's --rotate and --reflect options to get a rotated output, but wasn't able to generate negative x/y co-ordinates. I'm using linaro's debian userspace, running lxqt.

Hi,

Quoted 9 lines · Carsten Behling

modesetting X11 driver may provide negative x/y cordinates in mdp5_crtc_cursor_move call when rotation is enabled.

Cursor buffer can overlap down to its negative width/height.

ROI has to be recalculated for negative x/y indicating using the lower/right corner of the cursor buffer and hotspot must be set in MDP5_LM_CURSOR_XY_SRC_Y MDP5_LM_CURSOR_XY_SRC_X.

Thanks for the patch. Could you tell how to reproduce this issue on a db410c?

I was playing with xrandr's --rotate and --reflect options to get a rotated output, but wasn't able to generate negative x/y co-ordinates. I'm using linaro's debian userspace, running lxqt.

Thanks,
Archit

Quoted 118 lines

Signed-off-by Carsten Behling <[email protected]>

---
Changes in v2:
- fixed format specifier in debug message

  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 51
++++++++++++++++++++++++++-----
  1 file changed, 43 insertions(+), 8 deletions(-)

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +1 -1

index 10271359789e..a7f4a6688fec 100644
@@ -65,7 +65,7 @@ struct mdp5_crtc {
                struct drm_gem_object *scanout_bo;
                uint64_t iova;
                uint32_t width, height;
-               uint32_t x, y;
+               int x, y;
        } cursor;
  };
  #define to_mdp5_crtc(x) container_of(x, struct mdp5_crtc, base)

@@ -760,20 +760,31 @@ static void get_roi(struct drm_crtc *crtc, uint32_t
 *roi_w, uint32_t *roi_h)
         * Cursor Region Of Interest (ROI) is a plane read from cursor
         * buffer to render. The ROI region is determined by the

visibility of
* the cursor point. In the default Cursor image the cursor point
will

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +2 -2

-        * be at the top left of the cursor image, unless it is specified
-        * otherwise using hotspot feature.
+        * be at the top left of the cursor image.
         *
+        * Without rotation:
         * If the cursor point reaches the right (xres - x <

cursor.width) or
* bottom (yres - y < cursor.height) boundary of the screen, then
ROI
* width and ROI height need to be evaluated to crop the cursor
image
* accordingly.
* (xres-x) will be new cursor width when x > (xres -
cursor.width)
* (yres-y) will be new cursor height when y > (yres -
cursor.height)

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +9 -2

+        *
+        * With rotation:
+        * We get negative x and/or y coordinates.
+        * (cursor.width - abs(x)) will be new cursor width when x < 0
+        * (cursor.height - abs(y)) will be new cursor width when y < 0
         */
-       *roi_w = min(mdp5_crtc->cursor.width, xres -
+       if (mdp5_crtc->cursor.x >= 0)
+               *roi_w = min(mdp5_crtc->cursor.width, xres -
                        mdp5_crtc->cursor.x);
-       *roi_h = min(mdp5_crtc->cursor.height, yres -
+       else
+               *roi_w = mdp5_crtc->cursor.width -

abs(mdp5_crtc->cursor.x);

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +4 -0

+       if (mdp5_crtc->cursor.y >= 0)
+               *roi_h = min(mdp5_crtc->cursor.height, yres -
                        mdp5_crtc->cursor.y);
+       else
+               *roi_h = mdp5_crtc->cursor.height -

abs(mdp5_crtc->cursor.y);
}
static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +1 -1

@@ -783,7 +794,7 @@ static void mdp5_crtc_restore_cursor(struct drm_crtc
 *crtc)
        struct mdp5_kms *mdp5_kms = get_kms(crtc);
        const enum mdp5_cursor_alpha cur_alpha = CURSOR_ALPHA_PER_PIXEL;
        uint32_t blendcfg, stride;
-       uint32_t x, y, width, height;
+       uint32_t x, y, src_x, src_y, width, height;
        uint32_t roi_w, roi_h;
        int lm;

@@ -800,6 +811,26 @@ static void mdp5_crtc_restore_cursor(struct
drm_crtc *crtc)

        get_roi(crtc, &roi_w, &roi_h);

+ /* If cusror buffer overlaps due to rotation on the

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +19 -0

+        * upper or left screen border the pixel offset inside
+        * the cursor buffer of the ROI is the positive overlap
+        * distance.
+        */
+       if (mdp5_crtc->cursor.x < 0) {
+               src_x = abs(mdp5_crtc->cursor.x);
+               x = 0;
+       } else {
+               src_x = 0;
+       }
+       if (mdp5_crtc->cursor.y < 0) {
+               src_y = abs(mdp5_crtc->cursor.y);
+               y = 0;
+       } else {
+               src_y = 0;
+       }
+       DBG("%s: x=%u, y=%u roi_w=%u roi_h=%u src_x=%u src_y=%u",
+               crtc->name, x, y, roi_w, roi_h, src_x, src_y);
+
        mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm), stride);
        mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
                        MDP5_LM_CURSOR_FORMAT_FORMAT(C

URSOR_FMT_ARGB8888));

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +3 -0

@@ -812,6 +843,9 @@ static void mdp5_crtc_restore_cursor(struct drm_crtc
 *crtc)
        mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
                        MDP5_LM_CURSOR_START_XY_Y_START(y) |
                        MDP5_LM_CURSOR_START_XY_X_START(x));
+       mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_XY(lm),
+                       MDP5_LM_CURSOR_XY_SRC_Y(src_y) |
+                       MDP5_LM_CURSOR_XY_SRC_X(src_x));
        mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm),
                        mdp5_crtc->cursor.iova);

@@ -932,8 +966,9 @@ static int mdp5_crtc_cursor_move(struct drm_crtc
*crtc, int x, int y)

        if (unlikely(!crtc->state->enable))
                return 0;

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c static int mdp5_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) · line 973

Quoted 8 lines

- mdp5_crtc->cursor.x = x = max(x, 0);

drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c +3 -1

-       mdp5_crtc->cursor.y = y = max(y, 0);
+       /* accept negative x/y coordinates up to maximum cursor overlap */
+       mdp5_crtc->cursor.x = x = max(x, -(int)mdp5_crtc->cursor.width);
+       mdp5_crtc->cursor.y = y = max(y, -(int)mdp5_crtc->cursor.height);
        get_roi(crtc, &roi_w, &roi_h);

Hi,

Quoted 6 lines

Thanks for the patch. Could you tell how to reproduce this issue on a db410c?

I was playing with xrandr's --rotate and --reflect options to get a rotated output, but wasn't able to generate negative x/y co-ordinates. I'm using linaro's debian userspace, running lxqt.

I used Yocto Rocko from 96Boards

https://github.com/96boards/oe-rpb-manifest/tree/rocko

MACHINE=dragonboard-410c
DISTRO=rpb

rpb-desktop-image

Connect HDMI monitor and USB mouse, then

1.) Just boot. Wait for X-Server up.
2.) From my serial console:
     DISPLAY=:0.0 xrandr -o 2
3.) Try to move the mouse to the upper (the rotated lower) border.

Interesting to know that your debian user space is ok. The yocto X11 configuration is very basic. There may be a X11 configuration or extension that does the trick on Debian.

Therefore, I asked the X11 people where to fix:

https://www.spinics.net/lists/xorg/msg58969.html

Best regards
-Carsten

2018-07-24 19:33 GMT+02:00 Archit Taneja <[email protected] <mailto:[email protected]>>:

    Hi,

    On Tuesday 17 July 2018 04:33 AM, Carsten Behling wrote:

        modesetting X11 driver may provide negative x/y cordinates in
        mdp5_crtc_cursor_move call when rotation is enabled.

        Cursor buffer can overlap down to its negative width/height.

        ROI has to be recalculated for negative x/y indicating using the
        lower/right corner of the cursor buffer and hotspot must be set
        in MDP5_LM_CURSOR_XY_SRC_Y MDP5_LM_CURSOR_XY_SRC_X.


    Thanks for the patch. Could you tell how to reproduce this issue
    on a db410c?

    I was playing with xrandr's --rotate and --reflect options to get
    a rotated output, but wasn't able to generate negative x/y
    co-ordinates. I'm using linaro's debian userspace, running lxqt.

    Thanks,
    Archit



        Signed-off-by: Carsten Behling <[email protected]
        <mailto:[email protected]>>
        ---
        Changes in v2:
        - fixed format specifier in debug message

           drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 51
        ++++++++++++++++++++++++++-----
           1 file changed, 43 insertions(+), 8 deletions(-)

        diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
        b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
        index 10271359789e..a7f4a6688fec 100644
        --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
        +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
        @@ -65,7 +65,7 @@ struct mdp5_crtc {
                         struct drm_gem_object *scanout_bo;
                         uint64_t iova;
                         uint32_t width, height;
        -               uint32_t x, y;
        +               int x, y;
                 } cursor;
           };
           #define to_mdp5_crtc(x) container_of(x, struct mdp5_crtc, base)
        @@ -760,20 +760,31 @@ static void get_roi(struct drm_crtc *crtc,
        uint32_t *roi_w, uint32_t *roi_h)
                  * Cursor Region Of Interest (ROI) is a plane read from
        cursor
                  * buffer to render. The ROI region is determined by
        the visibility of
                  * the cursor point. In the default Cursor image the
        cursor point will
        -        * be at the top left of the cursor image, unless it is
        specified
        -        * otherwise using hotspot feature.
        +        * be at the top left of the cursor image.
                  *
        +        * Without rotation:
                  * If the cursor point reaches the right (xres - x <
        cursor.width) or
                  * bottom (yres - y < cursor.height) boundary of the
        screen, then ROI
                  * width and ROI height need to be evaluated to crop
        the cursor image
                  * accordingly.
                  * (xres-x) will be new cursor width when x > (xres -
        cursor.width)
                  * (yres-y) will be new cursor height when y > (yres -
        cursor.height)
        +        *
        +        * With rotation:
        +        * We get negative x and/or y coordinates.
        +        * (cursor.width - abs(x)) will be new cursor width when
        x < 0
        +        * (cursor.height - abs(y)) will be new cursor width
        when y < 0
                  */
        -       *roi_w = min(mdp5_crtc->cursor.width, xres -
        +       if (mdp5_crtc->cursor.x >= 0)
        +               *roi_w = min(mdp5_crtc->cursor.width, xres -
                                 mdp5_crtc->cursor.x);
        -       *roi_h = min(mdp5_crtc->cursor.height, yres -
        +       else
        +               *roi_w = mdp5_crtc->cursor.width -
        abs(mdp5_crtc->cursor.x);
        +       if (mdp5_crtc->cursor.y >= 0)
        +               *roi_h = min(mdp5_crtc->cursor.height, yres -
                                 mdp5_crtc->cursor.y);
        +       else
        +               *roi_h = mdp5_crtc->cursor.height -
        abs(mdp5_crtc->cursor.y);
           }
             static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
        @@ -783,7 +794,7 @@ static void mdp5_crtc_restore_cursor(struct
        drm_crtc *crtc)
                 struct mdp5_kms *mdp5_kms = get_kms(crtc);
                 const enum mdp5_cursor_alpha cur_alpha =
        CURSOR_ALPHA_PER_PIXEL;
                 uint32_t blendcfg, stride;
        -       uint32_t x, y, width, height;
        +       uint32_t x, y, src_x, src_y, width, height;
                 uint32_t roi_w, roi_h;
                 int lm;
           @@ -800,6 +811,26 @@ static void
        mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
                 get_roi(crtc, &roi_w, &roi_h);
           +     /* If cusror buffer overlaps due to rotation on the
        +        * upper or left screen border the pixel offset inside
        +        * the cursor buffer of the ROI is the positive overlap
        +        * distance.
        +        */
        +       if (mdp5_crtc->cursor.x < 0) {
        +               src_x = abs(mdp5_crtc->cursor.x);
        +               x = 0;
        +       } else {
        +               src_x = 0;
        +       }
        +       if (mdp5_crtc->cursor.y < 0) {
        +               src_y = abs(mdp5_crtc->cursor.y);
        +               y = 0;
        +       } else {
        +               src_y = 0;
        +       }
        +       DBG("%s: x=%u, y=%u roi_w=%u roi_h=%u src_x=%u src_y=%u",
        +               crtc->name, x, y, roi_w, roi_h, src_x, src_y);
        +
                 mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm),
        stride);
                 mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
                                
        MDP5_LM_CURSOR_FORMAT_FORMAT(CURSOR_FMT_ARGB8888));
        @@ -812,6 +843,9 @@ static void mdp5_crtc_restore_cursor(struct
        drm_crtc *crtc)
                 mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
                                 MDP5_LM_CURSOR_START_XY_Y_START(y) |
                                 MDP5_LM_CURSOR_START_XY_X_START(x));
        +       mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_XY(lm),
        +                       MDP5_LM_CURSOR_XY_SRC_Y(src_y) |
        +                       MDP5_LM_CURSOR_XY_SRC_X(src_x));
                 mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm),
                                 mdp5_crtc->cursor.iova);
           @@ -932,8 +966,9 @@ static int mdp5_crtc_cursor_move(struct
        drm_crtc *crtc, int x, int y)
                 if (unlikely(!crtc->state->enable))
                         return 0;
           -     mdp5_crtc->cursor.x = x = max(x, 0);

        -       mdp5_crtc->cursor.y = y = max(y, 0);
        +       /* accept negative x/y coordinates up to maximum cursor
        overlap */
        +       mdp5_crtc->cursor.x = x = max(x,
        -(int)mdp5_crtc->cursor.width);
        +       mdp5_crtc->cursor.y = y = max(y,
        -(int)mdp5_crtc->cursor.height);
                 get_roi(crtc, &roi_w, &roi_h);