drm/tegra: Add hardware cursor support
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
Quoted 5 lines · 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:
Quoted 8 lines
drivers/gpu/drm/tegra/dc.c:597 tegra_dc_cursor_set2() warn: mask and shift to zero
drivers/gpu/drm/tegra/dc.c594 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