Hi Thomas, Thanks for the detailed review, going through each point:
> Please use drm_device.dev instead. Will drop this field and use gdev->drm.dev everywhere instead. > Synchronization happens with drm_gem_fb_begin_cpu_access(). No need to > take the lock here. > > > + ret = drm_gem_shmem_vmap_locked(shmem, &map); > > This vmap should not happen here, but in begin_fb_access. We have > helpers that set up the mapping for your driver. Take a look at > DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and DRM_GEM_SHADOW_PLANE_FUNCS and the > drivers that use them. I wasn't aware these helpers existed, this looks like exactly the right fit for what I'm doing (software render into shmem, blit into hardware VRAM on flip). I'll switch to DRM_GEM_SHADOW_PLANE_HELPER_FUNCS/DRM_GEM_SHADOW_PLANE_FUNCS and drop the manual dma_resv_lock/vmap_locked handling entirely for v2. > That's confusing. Each pixel is 32 bit. But each color channel is only 4 > bit. So there's a 20-bit hole in each pixel, right? Why don't you use > 16 bit per pixel? You're right that it's wasteful as-is. One thing worth mentioning though, my VRAM write port on the hardware side is a fixed 32-bit Avalon-MM slave without byteenable support. The hardware itself always reads the lower 12 bits and ignores the rest, so that part is fine regardless of write width, but I'm not certain the interconnect would reliably place a 16-bit write into the lower half of the 32-bit word rather than, say, the upper half depending on address alignment, without byteenable support to make that explicit. I'm already planning to update the hardware to accept XRGB8888 natively, which will need the full 32-bit width regardless and removes the CPU-side pixel conversion entirely, so I'll leave this as a 32-bit write for now and address it as part of that hardware change rather than adding byteenable support for what would only be a temporary fix. > I don't think it is possible that new_state is NULL. I'll drop the redundant check. > No need for this function. drm_gem_fb_create_with_dirty() is exactly > what you're looking for. Thanks, I'll drop glanda_fb_create()/glanda_fb_funcs and just assign drm_gem_fb_create_with_dirty directly. > Drop these ioctls entirely. As I outlined in my reply to the cover > letter, these will not work with DRM as they are now. Especially since > you're using gem-shmem as a shadow buffer. All your rendering happens in > system memory in user space in software. Agreed, as mentioned in my reply to the cover letter, these are gone in v2. > This function is going away. You can open-code it instead. Will do, thanks for the heads up. I'll include all of this in v2 along with what we discussed on the cover letter thread. Thanks again for the thorough review. Best regards, Leander
