drm: hibmc: Initialize the hibmc_bo_driver.io_mem_pfn
Subsystems (4)
PATCH 1/1 drm: hibmc: Initialize the hibmc_bo_driver.io_mem_pfn
From: Michal Srb <[email protected]>
The io_mem_pfn field was added in ea642c3216cb2a60d1c0e760ae47ee85c9c16447 and is used unconditionally. Most drivers were updated to set it to the default implementation ttm_bo_default_io_mem_pfn, but hibmc was not.
This fixes crash in ttm_bo_vm_fault when hibmc driver is used.
--- drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c +1 -0
@@ -243,6 +243,7 @@ struct ttm_bo_driver hibmc_bo_driver = { .verify_access = hibmc_bo_verify_access, .io_mem_reserve = &hibmc_ttm_io_mem_reserve, .io_mem_free = NULL, + .io_mem_pfn = ttm_bo_default_io_mem_pfn, };int hibmc_mm_init (struct hibmc_drm_private *hibmc)
Signature · 6 lines
2.13.6 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 7 lines · On Wed, Nov 29, 2017 at 12:33:24PM +0100, [email protected] wrote:
From: Michal Srb <[email protected]>
The
io_mem_pfnfield was added inea642c3216cb2a60d1c0e760ae47ee85c9c16447and is used unconditionally. Most drivers were updated to set it to the default implementationttm_bo_default_io_mem_pfn, but hibmc was not.This fixes crash in
ttm_bo_vm_faultwhen hibmc driver is used.
Instead of just doing the same cargo cult in this driver, lets just call the default function when io_mem_pfn is NULL. Looks like 9 other drivers are affected as well.
Sean
Quoted 25 lines
Signed-off-by Michal Srb <[email protected]>--- drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c +1
-0@@ -243,6 +243,7 @@ struct ttm_bo_driver hibmc_bo_driver = { .verify_access = hibmc_bo_verify_access, .io_mem_reserve = &hibmc_ttm_io_mem_reserve, .io_mem_free = NULL, + .io_mem_pfn = ttm_bo_default_io_mem_pfn, };int hibmc_mm_init (struct hibmc_drm_private *hibmc)Signature · 6 lines
2.13.6 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 7 lines · On středa 29. listopadu 2017 15:47:30 CET Sean Paul wrote:
Quoted 6 lines · On Wed, Nov 29, 2017 at 12:33:24PM +0100, [email protected] wrote:
From: Michal Srb <[email protected]>
The
io_mem_pfnfield was added inea642c3216cb2a60d1c0e760ae47ee85c9c16447and is used unconditionally. Most drivers were updated to set it to the default implementationttm_bo_default_io_mem_pfn, but hibmc was not.
Quoted 5 lines
Quoted 1 line
This fixes crash in
ttm_bo_vm_faultwhen hibmc driver is used.Instead of just doing the same cargo cult in this driver, lets just call the default function when
io_mem_pfnis NULL. Looks like 9 other drivers are affected as well.
Ok, that makes sense.
Michal
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/1 drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
In reply to Sean PaulThe 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.
--- drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/gpu/drm/ttm/ttm_bo_vm.c +4 -1
@@ -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 )) {
Signature · 6 lines
2.13.6 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Christian König
NAKRe: [PATCH] drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
Am 29.11.2017 um 16:20 schrieb Michal Srb:
Quoted 6 lines
The
io_mem_pfnfield was added in commitea642c3216cb2a60d1c0e760ae47ee85c9c16447and is called unconditionally. However, not all drivers were updated to set it.Use the
ttm_bo_default_io_mem_pfnfunction if a driver did not set its own.Signed-off-by Michal Srb <[email protected]>
NAK, when we have drivers missing this we should set this in the driver and not add the workaround here.
Which driver is missing this? I thought I fixed all of them.
Regards,
Christian.
Quoted 10 lines
--- drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpu/drm/ttm/ttm_bo_vm.c +0
-0@@ -234,7 +234,10 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)if (bo->mem.bus.is_iomem) {
drivers/gpu/drm/ttm/ttm_bo_vm.c static int ttm_bo_vm_fault(struct vm_fault *vmf) · line 243
Quoted 10 lines
drivers/gpu/drm/ttm/ttm_bo_vm.c +4
-1/* 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
Quoted 8 lines · On Wednesday 29th November 2017 16:24:21 CET Christian König wrote:
Am 29.11.2017 um 16:20 schrieb Michal Srb:
Quoted 7 lines
The
io_mem_pfnfield was added in commitea642c3216cb2a60d1c0e760ae47ee85c9c16447and is called unconditionally. However, not all drivers were updated to set it.Use the
ttm_bo_default_io_mem_pfnfunction if a driver did not set its own.
Quoted 6 lines
Quoted 1 line
Signed-off-by Michal Srb <[email protected]>NAK, when we have drivers missing this we should set this in the driver and not add the workaround here.
Which driver is missing this? I thought I fixed all of them.
The one where I ran into the problem is hibmc. I've sent patch fixing it, but reportedly there is more drivers with the same issue, so this fix was suggested instead:
Quoted 5 lines · On Wednesday 29th November 2017 15:47:30 CET Sean Paul wrote:
Instead of just doing the same cargo cult in this driver, lets just call the default function when
io_mem_pfnis NULL. Looks like 9 other drivers are affected as well.Sean
Michal
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
In reply to Christian KönigQuoted 10 lines · Christian König
Am 29.11.2017 um 16:20 schrieb Michal Srb:
Quoted 6 lines
The
io_mem_pfnfield was added in commitea642c3216cb2a60d1c0e760ae47ee85c9c16447and is called unconditionally. However, not all drivers were updated to set it.Use the
ttm_bo_default_io_mem_pfnfunction if a driver did not set its own.Signed-off-by Michal Srb <[email protected]>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)?
Sean
Quoted 31 lines
Which driver is missing this? I thought I fixed all of them.
Regards,
Christian.Quoted 10 lines
--- drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpu/drm/ttm/ttm_bo_vm.c +0
-0@@ -234,7 +234,10 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)if (bo->mem.bus.is_iomem) {drivers/gpu/drm/ttm/ttm_bo_vm.c static int ttm_bo_vm_fault(struct vm_fault *vmf) · line 243
Quoted 10 lines
drivers/gpu/drm/ttm/ttm_bo_vm.c +4
-1/* 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
Am 29.11.2017 um 16:36 schrieb Sean Paul:
Quoted 5 lines · Christian König
Quoted 4 lines
Am 29.11.2017 um 16:20 schrieb Michal Srb:
Quoted 3 lines
The
io_mem_pfnfield was added in commitea642c3216cb2a60d1c0e760ae47ee85c9c16447and is called unconditionally. However, not all drivers were updated to set it.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Use the
ttm_bo_default_io_mem_pfnfunction if a driver did not set its own.Signed-off-by Michal Srb <[email protected]>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_pfnfrom those as well)?
Not have this extra check in common TTM code? That is seriously bad coding style.
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.
Quoted 32 lines
Sean
Quoted 29 lines
Which driver is missing this? I thought I fixed all of them.
Regards,
Christian.Quoted 10 lines
--- drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpu/drm/ttm/ttm_bo_vm.c +0
-0@@ -234,7 +234,10 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)if (bo->mem.bus.is_iomem) {drivers/gpu/drm/ttm/ttm_bo_vm.c static int ttm_bo_vm_fault(struct vm_fault *vmf) · line 243
Quoted 10 lines
drivers/gpu/drm/ttm/ttm_bo_vm.c +4
-1/* 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
Quoted 9 lines · Christian König
Am 29.11.2017 um 16:36 schrieb Sean Paul:
Quoted 8 lines · Christian König
Quoted 7 lines
Am 29.11.2017 um 16:20 schrieb Michal Srb:
Quoted 6 lines
The
io_mem_pfnfield was added in commitea642c3216cb2a60d1c0e760ae47ee85c9c16447and is called unconditionally. However, not all drivers were updated to set it.Use the
ttm_bo_default_io_mem_pfnfunction if a driver did not set its own.Signed-off-by Michal Srb <[email protected]>
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
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_pfnfrom 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 :-)
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.
Sean
Quoted 40 lines
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.
Quoted 32 lines
Sean
Quoted 29 lines
Which driver is missing this? I thought I fixed all of them.
Regards,
Christian.Quoted 10 lines
--- drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpu/drm/ttm/ttm_bo_vm.c +0
-0@@ -234,7 +234,10 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)if (bo->mem.bus.is_iomem) {drivers/gpu/drm/ttm/ttm_bo_vm.c static int ttm_bo_vm_fault(struct vm_fault *vmf) · line 243
Quoted 10 lines
drivers/gpu/drm/ttm/ttm_bo_vm.c +4
-1/* 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
Am 29.11.2017 um 16:44 schrieb Sean Paul:
Quoted 15 lines · Christian König
Quoted 14 lines
Am 29.11.2017 um 16:36 schrieb Sean Paul:
Quoted 13 lines · Christian König
Quoted 9 lines
Am 29.11.2017 um 16:20 schrieb Michal Srb:
Quoted 6 lines
The
io_mem_pfnfield was added in commitea642c3216cb2a60d1c0e760ae47ee85c9c16447and is called unconditionally. However, not all drivers were updated to set it.Use the
ttm_bo_default_io_mem_pfnfunction if a driver did not set its own.Signed-off-by Michal Srb <[email protected]>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_pfnfrom those as well)?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 42 lines
Sean
Quoted 38 lines
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.
Quoted 31 lines
Sean
Quoted 29 lines
Which driver is missing this? I thought I fixed all of them.
Regards,
Christian.Quoted 10 lines
--- drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpu/drm/ttm/ttm_bo_vm.c +0
-0@@ -234,7 +234,10 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)if (bo->mem.bus.is_iomem) {drivers/gpu/drm/ttm/ttm_bo_vm.c static int ttm_bo_vm_fault(struct vm_fault *vmf) · line 243
Quoted 10 lines
drivers/gpu/drm/ttm/ttm_bo_vm.c +4
-1/* 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: Use ttm_bo_default_io_mem_pfn if io_mem_pfn is NULL
Quoted 45 lines · Christian König
Am 29.11.2017 um 16:44 schrieb Sean Paul:
Quoted 15 lines · Christian König
Quoted 14 lines
Am 29.11.2017 um 16:36 schrieb Sean Paul:
Quoted 13 lines · Christian König
Quoted 9 lines
Am 29.11.2017 um 16:20 schrieb Michal Srb:
Quoted 6 lines
The
io_mem_pfnfield was added in commitea642c3216cb2a60d1c0e760ae47ee85c9c16447and is called unconditionally. However, not all drivers were updated to set it.Use the
ttm_bo_default_io_mem_pfnfunction if a driver did not set its own.Signed-off-by Michal Srb <[email protected]>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_pfnfrom those as well)?Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
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 */
Quoted 5 lines
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.
Yeah, that's what I was asking Michal for. I'd like a patch series where:
[0/N] adds the inline to call the default if io_mem_pfn is NULL [1..N-1/N] remove all .io_mem_pfn = ttm_bo_default_io_mem_pfn from drivers [N/N] unexport ttm_bo_default_io_mem_pfn and make it static
Sean
Quoted 46 lines
Christian.
Quoted 42 lines
Sean
Quoted 38 lines
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.
Quoted 31 lines
Sean
Quoted 29 lines
Which driver is missing this? I thought I fixed all of them.
Regards,
Christian.Quoted 10 lines
--- drivers/gpu/drm/ttm/ttm_bo_vm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/gpu/drm/ttm/ttm_bo_vm.c +0
-0@@ -234,7 +234,10 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)if (bo->mem.bus.is_iomem) {drivers/gpu/drm/ttm/ttm_bo_vm.c static int ttm_bo_vm_fault(struct vm_fault *vmf) · line 243
Quoted 10 lines
drivers/gpu/drm/ttm/ttm_bo_vm.c +4
-1/* 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel