linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/msm/display: negative x/y in cursor move
@ 2018-07-16 17:49 Carsten Behling
  2018-07-16 22:06 ` kbuild test robot
  0 siblings, 1 reply; 5+ messages in thread
From: Carsten Behling @ 2018-07-16 17:49 UTC (permalink / raw)
  Cc: Carsten Behling, Rob Clark, David Airlie, Archit Taneja,
	Sean Paul, Steve Kowalik, Daniel Vetter, Viresh Kumar,
	linux-arm-msm, dri-devel, freedreno, linux-kernel

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 <carsten.behling@gmail.com>
---
 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..43a86582876c 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=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
+		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);
 
-- 
2.14.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/msm/display: negative x/y in cursor move
  2018-07-16 17:49 [PATCH] drm/msm/display: negative x/y in cursor move Carsten Behling
@ 2018-07-16 22:06 ` kbuild test robot
  2018-07-16 23:03   ` [PATCH v2] " Carsten Behling
  0 siblings, 1 reply; 5+ messages in thread
From: kbuild test robot @ 2018-07-16 22:06 UTC (permalink / raw)
  To: Carsten Behling
  Cc: kbuild-all, Carsten Behling, Rob Clark, David Airlie,
	Archit Taneja, Sean Paul, Steve Kowalik, Daniel Vetter,
	Viresh Kumar, linux-arm-msm, dri-devel, freedreno, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5858 bytes --]

Hi Carsten,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robclark/msm-next]
[also build test WARNING on v4.18-rc5 next-20180716]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Carsten-Behling/drm-msm-display-negative-x-y-in-cursor-move/20180717-031351
base:   git://people.freedesktop.org/~robclark/linux msm-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm64 

All warnings (new ones prefixed by >>):

   In file included from include/drm/drm_mm.h:49:0,
                    from include/drm/drmP.h:73,
                    from include/drm/drm_modeset_helper.h:26,
                    from include/drm/drm_crtc_helper.h:44,
                    from drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:22:
   drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c: In function 'mdp5_crtc_restore_cursor':
>> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:831:6: warning: format '%s' expects argument of type 'char *', but argument 3 has type 'uint32_t {aka unsigned int}' [-Wformat=]
     DBG("%s: x=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
         ^
   include/drm/drm_print.h:285:25: note: in definition of macro 'DRM_DEBUG_DRIVER'
     drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
                            ^~~
>> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:831:2: note: in expansion of macro 'DBG'
     DBG("%s: x=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
     ^~~
   drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:831:8: note: format string is defined here
     DBG("%s: x=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
          ~^
          %d
   In file included from include/drm/drm_mm.h:49:0,
                    from include/drm/drmP.h:73,
                    from include/drm/drm_modeset_helper.h:26,
                    from include/drm/drm_crtc_helper.h:44,
                    from drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:22:
>> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:831:6: warning: format '%d' expects a matching 'int' argument [-Wformat=]
     DBG("%s: x=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
         ^
   include/drm/drm_print.h:285:25: note: in definition of macro 'DRM_DEBUG_DRIVER'
     drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
                            ^~~
>> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:831:2: note: in expansion of macro 'DBG'
     DBG("%s: x=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
     ^~~
   drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c:831:56: note: format string is defined here
     DBG("%s: x=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
                                                          ~^

vim +831 drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c

   789	
   790	static void mdp5_crtc_restore_cursor(struct drm_crtc *crtc)
   791	{
   792		struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
   793		struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
   794		struct mdp5_kms *mdp5_kms = get_kms(crtc);
   795		const enum mdp5_cursor_alpha cur_alpha = CURSOR_ALPHA_PER_PIXEL;
   796		uint32_t blendcfg, stride;
   797		uint32_t x, y, src_x, src_y, width, height;
   798		uint32_t roi_w, roi_h;
   799		int lm;
   800	
   801		assert_spin_locked(&mdp5_crtc->cursor.lock);
   802	
   803		lm = mdp5_cstate->pipeline.mixer->lm;
   804	
   805		x = mdp5_crtc->cursor.x;
   806		y = mdp5_crtc->cursor.y;
   807		width = mdp5_crtc->cursor.width;
   808		height = mdp5_crtc->cursor.height;
   809	
   810		stride = width * drm_format_plane_cpp(DRM_FORMAT_ARGB8888, 0);
   811	
   812		get_roi(crtc, &roi_w, &roi_h);
   813	
   814		/* If cusror buffer overlaps due to rotation on the
   815		 * upper or left screen border the pixel offset inside
   816		 * the cursor buffer of the ROI is the positive overlap
   817		 * distance.
   818		 */
   819		if (mdp5_crtc->cursor.x < 0) {
   820			src_x = abs(mdp5_crtc->cursor.x);
   821			x = 0;
   822		} else {
   823			src_x = 0;
   824		}
   825		if (mdp5_crtc->cursor.y < 0) {
   826			src_y = abs(mdp5_crtc->cursor.y);
   827			y = 0;
   828		} else {
   829			src_y = 0;
   830		}
 > 831		DBG("%s: x=%d, y=%d roi_w=%d roi_h=%d src_x=%d src_y=%d",
   832			x, y, roi_w, roi_h, src_x, src_y);
   833	
   834		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_STRIDE(lm), stride);
   835		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_FORMAT(lm),
   836				MDP5_LM_CURSOR_FORMAT_FORMAT(CURSOR_FMT_ARGB8888));
   837		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_IMG_SIZE(lm),
   838				MDP5_LM_CURSOR_IMG_SIZE_SRC_H(height) |
   839				MDP5_LM_CURSOR_IMG_SIZE_SRC_W(width));
   840		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_SIZE(lm),
   841				MDP5_LM_CURSOR_SIZE_ROI_H(roi_h) |
   842				MDP5_LM_CURSOR_SIZE_ROI_W(roi_w));
   843		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_START_XY(lm),
   844				MDP5_LM_CURSOR_START_XY_Y_START(y) |
   845				MDP5_LM_CURSOR_START_XY_X_START(x));
   846		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_XY(lm),
   847				MDP5_LM_CURSOR_XY_SRC_Y(src_y) |
   848				MDP5_LM_CURSOR_XY_SRC_X(src_x));
   849		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BASE_ADDR(lm),
   850				mdp5_crtc->cursor.iova);
   851	
   852		blendcfg = MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_EN;
   853		blendcfg |= MDP5_LM_CURSOR_BLEND_CONFIG_BLEND_ALPHA_SEL(cur_alpha);
   854		mdp5_write(mdp5_kms, REG_MDP5_LM_CURSOR_BLEND_CONFIG(lm), blendcfg);
   855	}
   856	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38925 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] drm/msm/display: negative x/y in cursor move
  2018-07-16 22:06 ` kbuild test robot
@ 2018-07-16 23:03   ` Carsten Behling
  2018-07-24 17:33     ` Archit Taneja
  0 siblings, 1 reply; 5+ messages in thread
From: Carsten Behling @ 2018-07-16 23:03 UTC (permalink / raw)
  Cc: Carsten Behling, Rob Clark, David Airlie, Archit Taneja,
	Sean Paul, Daniel Vetter, Maarten Lankhorst, Steve Kowalik,
	Viresh Kumar, linux-arm-msm, dri-devel, freedreno, linux-kernel

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 <carsten.behling@gmail.com>
---
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);
 
-- 
2.14.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] drm/msm/display: negative x/y in cursor move
  2018-07-16 23:03   ` [PATCH v2] " Carsten Behling
@ 2018-07-24 17:33     ` Archit Taneja
       [not found]       ` <CAPuGWB9oBN4U6OS8FLDQ1m6eO3n4mT-UG3QWHdjuM9XgSLG8iQ@mail.gmail.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Archit Taneja @ 2018-07-24 17:33 UTC (permalink / raw)
  To: Carsten Behling
  Cc: Carsten Behling, Rob Clark, David Airlie, Sean Paul,
	Daniel Vetter, Maarten Lankhorst, Steve Kowalik, Viresh Kumar,
	linux-arm-msm, dri-devel, freedreno, linux-kernel

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 <carsten.behling@gmail.com>
> ---
> 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);
>   
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] drm/msm/display: negative x/y in cursor move
       [not found]       ` <CAPuGWB9oBN4U6OS8FLDQ1m6eO3n4mT-UG3QWHdjuM9XgSLG8iQ@mail.gmail.com>
@ 2018-07-26  7:25         ` Archit Taneja
  0 siblings, 0 replies; 5+ messages in thread
From: Archit Taneja @ 2018-07-26  7:25 UTC (permalink / raw)
  To: Carsten Behling
  Cc: Carsten Behling, Rob Clark, David Airlie, Sean Paul,
	Daniel Vetter, Maarten Lankhorst, Steve Kowalik, Viresh Kumar,
	linux-arm-msm, dri-devel, freedreno, linux-kernel



On Wednesday 25 July 2018 08:40 PM, Carsten Behling wrote:
> Hi,
> 
>> 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.

Thanks, I'll give this a try.

The patch looks good, anyway. Rob's queued it for msm-next.

Archit

> 
> 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 <architt@codeaurora.org 
> <mailto:architt@codeaurora.org>>:
> 
>     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 <carsten.behling@gmail.com
>         <mailto:carsten.behling@gmail.com>>
>         ---
>         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);
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-07-26  7:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-16 17:49 [PATCH] drm/msm/display: negative x/y in cursor move Carsten Behling
2018-07-16 22:06 ` kbuild test robot
2018-07-16 23:03   ` [PATCH v2] " Carsten Behling
2018-07-24 17:33     ` Archit Taneja
     [not found]       ` <CAPuGWB9oBN4U6OS8FLDQ1m6eO3n4mT-UG3QWHdjuM9XgSLG8iQ@mail.gmail.com>
2018-07-26  7:25         ` Archit Taneja

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).