Skip to contentSkip to conversation

drm/tegra: Add hardware cursor support

DISCUSSION Dan Carpenter 2 participants 2 messages Oct 17, 2014 Tegra architecture support

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw
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.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