re: drm/tegra: Add hardware cursor support

re: drm/tegra: Add hardware cursor support

[Dan Carpenter]

Hello Thierry Reding,

The patch e687651bc1ed: "drm/tegra: Add hardware cursor support" from
Dec 20, 2013, leads to the following static checker warning:

	drivers/gpu/drm/tegra/dc.c:597 tegra_dc_cursor_set2()
	warn: mask and shift to zero

drivers/gpu/drm/tegra/dc.c
   594          if (bo) {
   595                  unsigned long addr = (bo->paddr & 0xfffffc00) >> 10;
   596  #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
   597                  unsigned long high = (bo->paddr & 0xfffffffc) >> 32;
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I'm not sure what the correct mask is.

   598  #endif
   599  

regards,
dan carpenter

Re: drm/tegra: Add hardware cursor support

[Thierry Reding]

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

On Fri, Oct 17, 2014 at 02:31:23PM +0300, Dan Carpenter wrote:
> Hello Thierry Reding,
> 
> The patch e687651bc1ed: "drm/tegra: Add hardware cursor support" from
> Dec 20, 2013, leads to the following static checker warning:
> 
> 	drivers/gpu/drm/tegra/dc.c:597 tegra_dc_cursor_set2()
> 	warn: mask and shift to zero
> 
> drivers/gpu/drm/tegra/dc.c
>    594          if (bo) {
>    595                  unsigned long addr = (bo->paddr & 0xfffffc00) >> 10;
>    596  #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>    597                  unsigned long high = (bo->paddr & 0xfffffffc) >> 32;

How timely. I'm in fact revising this code at this very moment. =)

The masking here is indeed somewhat fishy. While it does the right
thing, it's also needless to some degree. A better version would be:

	addr = (bo->paddr >> 10) & 0x3fffff;

Since this is written to a register where the upper 10 bits (actually
only 8 because bits 22 and 23 are reserved) are used for additional
control parameters (cursor size and clipping region). The above will
properly write bits 31:10 into bits 21:0 of addr.

For the upper 32 bits the mask is completely redundant. According to the
register documentation, bits 1:0 should correspond to bits 33:32 of the
physical address, so something like:

	high = (bo->paddr >> 32) & 0x3;

would be more correct.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]