Am 29.11.2017 um 16:44 schrieb Sean Paul:
On Wed, Nov 29, 2017 at 04:40:18PM +0100, Christian König wrote:
Am 29.11.2017 um 16:36 schrieb Sean Paul:
On Wed, Nov 29, 2017 at 04:24:21PM +0100, Christian König wrote:
Am 29.11.2017 um 16:20 schrieb Michal Srb:
The io_mem_pfn field was added in commit ea642c3216cb2a60d1c0e760ae47ee85c9c16447
and is called unconditionally. However, not all drivers were updated to set it.
Use the ttm_bo_default_io_mem_pfn function if a driver did not set its own.
Signed-off-by: Michal Srb <msrb@xxxxxxxx>
NAK, when we have drivers missing this we should set this in the driver and
not add the workaround here.
Why? What's the benefit in adding the same hook in 10 drivers (I was hoping
Michal would also remove the token .io_mem_pfn = ttm_bo_default_io_mem_pfn from
those as well)?
Not have this extra check in common TTM code? That is seriously bad coding
style.
[citation neeeded]
I'd argue it's bad form to require everyone add a copypasta hook to their driver
everytime an unrelated driver needs more control than the default gives :-)
Well that is another design problem of TTM that it works like a middle
layer instead of a tool. But I'm seriously not going to fix this anytime
soon.
I think your point would hold if we were talking about a couple of drivers, but
once you scale to 10+, it's no longer a trivial amount of code. If the null check
is too distracting, you could always pull it out into an inline.
Yeah, that is one possibility that would work for me as well.
TTM has patterns like the following:
First occasion:
if (a_is_available)
do_a();
else
do_b();
Same code a bit lower:
if (a_is_available)
do_a()
/* Else is missing here */
I'm seriously favoring updating a dozen drivers just to avoid more of this.
If you want to fix that with adding wrapper functions to call the driver
callback and give a consistent operation when the callback isn't
available then I'm perfectly fine with that as well. Patches are always
welcome.
Christian.
Sean
We have tons of fallbacks like this and I unfortunately even created new one
without realizing that this is a problem.
But this really clutters the TTM code and makes it harder to understand.
Christian.
Sean
Which driver is missing this? I thought I fixed all of them.
Regards,
Christian.
---
drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index c8ebb757e36b..e25a99bc519d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -234,7 +234,10 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)
if (bo->mem.bus.is_iomem) {
/* Iomem should not be marked encrypted */
cvma.vm_page_prot = pgprot_decrypted(cvma.vm_page_prot);
- pfn = bdev->driver->io_mem_pfn(bo, page_offset);
+ if (bdev->driver->io_mem_pfn)
+ pfn = bdev->driver->io_mem_pfn(bo, page_offset);
+ else
+ pfn = ttm_bo_default_io_mem_pfn(bo, page_offset);
} else {
page = ttm->pages[page_offset];
if (unlikely(!page && i == 0)) {
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
