On Tue, Mar 23, 2021 at 10:05:23PM +0300, Dmitry Osipenko wrote:

23.03.2021 21:24, Thierry Reding пишет:

quoted

On Tue, Mar 23, 2021 at 08:57:42PM +0300, Dmitry Osipenko wrote:

quoted

23.03.2021 18:54, Thierry Reding пишет:

quoted

@@ -920,15 +934,42 @@ static void tegra_cursor_atomic_update(struct drm_plane *plane,
 	value = tegra_dc_readl(dc, DC_DISP_BLEND_CURSOR_CONTROL);
 	value &= ~CURSOR_DST_BLEND_MASK;
 	value &= ~CURSOR_SRC_BLEND_MASK;
-	value |= CURSOR_MODE_NORMAL;
+
+	if (dc->soc->has_nvdisplay)
+		value &= ~CURSOR_COMPOSITION_MODE_XOR;
+	else
+		value |= CURSOR_MODE_NORMAL;
+
 	value |= CURSOR_DST_BLEND_NEG_K1_TIMES_SRC;
 	value |= CURSOR_SRC_BLEND_K1_TIMES_SRC;
 	value |= CURSOR_ALPHA;
 	tegra_dc_writel(dc, value, DC_DISP_BLEND_CURSOR_CONTROL);
 
+	/* nvdisplay relies on software for clipping */
+	if (dc->soc->has_nvdisplay) {

But coordinates already should be clipped by
drm_atomic_helper_check_plane_state().

Yes, and the driver goes on to use the clipped coordinates later on in
this function.

I see now what it does, looks okay.

Minor nit: the i/j aren't very expressive names, something like sx/sy
sw/sh should be a bit more appropriate naming.

You could also make use of drm_rect_width/height helpers.

But this doesn't deserve a v2 if there is nothing more important to improve.

As I was browsing through the drm_rect helpers, I also came across
drm_rect_fp_to_int(), which helps simplify this further. As a result I
was able to just get rid of most of these temporary variables in favor
of just the integer version of drm_rect.

Thanks for the hint.

Thierry