Re: [PATCH 4/9] drm/tegra: dc: Implement hardware cursor on Tegra186 and later

[Thierry Reding <thierry.reding@gmail.com>]

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

On Tue, Mar 23, 2021 at 10:05:23PM +0300, Dmitry Osipenko wrote:
> 23.03.2021 21:24, Thierry Reding пишет:
> > On Tue, Mar 23, 2021 at 08:57:42PM +0300, Dmitry Osipenko wrote:
> >> 23.03.2021 18:54, Thierry Reding пишет:
> >>> @@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]