Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c
> On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote: > > On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote: > > > Peilin Ye (5): > > > console: Delete unused con_font_copy() callback implementations > > > console: Delete dummy con_font_set() and con_font_default() callback > > > implementations > > > Fonts: Add charcount field to font_desc > > > parisc/sticore: Avoid hard-coding built-in font charcount > > > fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount > > > > Patches all look good to me, if Greg is ok with me applying the entire > > pile to drm-misc-next I'll do that next week. On Fri, Nov 13, 2020 at 11:47:51PM +0100, Greg Kroah-Hartman wrote: > Reviewed-by: Greg Kroah-Hartman Thanks for reviewing! Questions about the last patch [5/5] though, it depends on the following assumption: """ For each console `idx`, `vc_cons[idx].d->vc_font.data` and `fb_display[idx].fontdata` always point to the same buffer. """ Is this true? I think it is true by grepping for `fontdata`. I also noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata` interchangeably, see fbcon_get_requirement(): vc = vc_cons[fg_console].d; [...] p = &fb_display[fg_console]; caps->x = 1 << (vc->vc_font.width - 1); ^^^ caps->y = 1 << (vc->vc_font.height - 1); ^^^ caps->len = (p->userfont) ? FNTCHARCNT(p->fontdata) : 256; ^^^ If it is true, then what is the point of using `fontdata` in `struct fbcon_display`? Just for the `userfont` flag? Should we delete `fontdata`, when we no longer need the `userfont` flag? In this sense I think [5/5] needs more testing. Do we have test files for fbcon, or should I try to write some tests from scratch? Thank you, Peilin Ye ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/8] drm: Add dummy page per device or GEM object
Am 13.11.20 um 21:52 schrieb Andrey Grodzovsky:
On 6/22/20 1:50 PM, Daniel Vetter wrote:
On Mon, Jun 22, 2020 at 7:45 PM Christian König
wrote:
Am 22.06.20 um 16:32 schrieb Andrey Grodzovsky:
On 6/22/20 9:18 AM, Christian König wrote:
Am 21.06.20 um 08:03 schrieb Andrey Grodzovsky:
Will be used to reroute CPU mapped BO's page faults once
device is removed.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/drm_file.c | 8
drivers/gpu/drm/drm_prime.c | 10 ++
include/drm/drm_file.h | 2 ++
include/drm/drm_gem.h | 2 ++
4 files changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index c4c704e..67c0770 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -188,6 +188,12 @@ struct drm_file *drm_file_alloc(struct
drm_minor *minor)
goto out_prime_destroy;
}
+ file->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+ if (!file->dummy_page) {
+ ret = -ENOMEM;
+ goto out_prime_destroy;
+ }
+
return file;
out_prime_destroy:
@@ -284,6 +290,8 @@ void drm_file_free(struct drm_file *file)
if (dev->driver->postclose)
dev->driver->postclose(dev, file);
+ __free_page(file->dummy_page);
+
drm_prime_destroy_file_private(&file->prime);
WARN_ON(!list_empty(&file->event_list));
diff --git a/drivers/gpu/drm/drm_prime.c
b/drivers/gpu/drm/drm_prime.c
index 1de2cde..c482e9c 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -335,6 +335,13 @@ int drm_gem_prime_fd_to_handle(struct
drm_device *dev,
ret = drm_prime_add_buf_handle(&file_priv->prime,
dma_buf, *handle);
+
+ if (!ret) {
+ obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+ if (!obj->dummy_page)
+ ret = -ENOMEM;
+ }
+
While the per file case still looks acceptable this is a clear NAK
since it will massively increase the memory needed for a prime
exported object.
I think that this is quite overkill in the first place and for the
hot unplug case we can just use the global dummy page as well.
Christian.
Global dummy page is good for read access, what do you do on write
access ? My first approach was indeed to map at first global dummy
page as read only and mark the vma->vm_flags as !VM_SHARED assuming
that this would trigger Copy On Write flow in core mm
(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7-rc7%2Fsource%2Fmm%2Fmemory.c%23L3977&data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7C3753451d037544e7495408d816d4c4ee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637284450384586120&sdata=ZpRaQgqA5K4jRfidOiedey0AleeYQ97WNUkGA29ERA0%3D&reserved=0)
on the next page fault to same address triggered by a write access but
then i realized a new COW page will be allocated for each such mapping
and this is much more wasteful then having a dedicated page per GEM
object.
Yeah, but this is only for a very very small corner cases. What we need
to prevent is increasing the memory usage during normal operation to
much.
Using memory during the unplug is completely unproblematic because we
just released quite a bunch of it by releasing all those system memory
buffers.
And I'm pretty sure that COWed pages are correctly accounted towards
the
used memory of a process.
So I think if that approach works as intended and the COW pages are
released again on unmapping it would be the perfect solution to the
problem.
Daniel what do you think?
If COW works, sure sounds reasonable. And if we can make sure we
managed to drop all the system allocations (otherwise suddenly 2x
memory usage, worst case). But I have no idea whether we can
retroshoehorn that into an established vma, you might have fun stuff
like a mkwrite handler there (which I thought is the COW handler
thing, but really no idea).
If we need to massively change stuff then I think rw dummy page,
allocated on first fault after hotunplug (maybe just make it one per
object, that's simplest) seems like the much safer option. Much less
code that can go wrong.
-Daniel
Regarding COW, i was looking into how to properly implement it from
within the fault handler (i.e. ttm_bo_vm_fault)
and the main obstacle I hit is that of exclusive access to the
vm_area_struct, i need to be able to modify
vma->vm_flags (and vm_page_prot) to remove VM_SHARED bit so COW can
be triggered on subsequent write access
fault (here
https://elixir.bootlin.com/linux/latest/source/mm/memory.c#L4128)
but core mm takes only read side mm_sem (here for example
https://elixir.bootlin.com/linux/latest/source/drivers/iommu/amd/iommu_v2.c#L488)
and so I am not supposed to modify vm_area_struct in this case. I am
not sure if it's legit to write lock tthe mm_sem from this point.
I found some discussions about this here
http://lkml.iu.edu/hypermail/linux/kernel/1909.1/02754.html but it
wasn't really clear to me
wha
Re: [PATCH v2 1/8] drm: Add dummy page per device or GEM object
On Sat, Nov 14, 2020 at 9:41 AM Christian König
wrote:
>
> Am 13.11.20 um 21:52 schrieb Andrey Grodzovsky:
> >
> > On 6/22/20 1:50 PM, Daniel Vetter wrote:
> >> On Mon, Jun 22, 2020 at 7:45 PM Christian König
> >> wrote:
> >>> Am 22.06.20 um 16:32 schrieb Andrey Grodzovsky:
> On 6/22/20 9:18 AM, Christian König wrote:
> > Am 21.06.20 um 08:03 schrieb Andrey Grodzovsky:
> >> Will be used to reroute CPU mapped BO's page faults once
> >> device is removed.
> >>
> >> Signed-off-by: Andrey Grodzovsky
> >> ---
> >>drivers/gpu/drm/drm_file.c | 8
> >>drivers/gpu/drm/drm_prime.c | 10 ++
> >>include/drm/drm_file.h | 2 ++
> >>include/drm/drm_gem.h | 2 ++
> >>4 files changed, 22 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> >> index c4c704e..67c0770 100644
> >> --- a/drivers/gpu/drm/drm_file.c
> >> +++ b/drivers/gpu/drm/drm_file.c
> >> @@ -188,6 +188,12 @@ struct drm_file *drm_file_alloc(struct
> >> drm_minor *minor)
> >>goto out_prime_destroy;
> >>}
> >>+file->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> >> +if (!file->dummy_page) {
> >> +ret = -ENOMEM;
> >> +goto out_prime_destroy;
> >> +}
> >> +
> >>return file;
> >> out_prime_destroy:
> >> @@ -284,6 +290,8 @@ void drm_file_free(struct drm_file *file)
> >>if (dev->driver->postclose)
> >>dev->driver->postclose(dev, file);
> >>+__free_page(file->dummy_page);
> >> +
> >> drm_prime_destroy_file_private(&file->prime);
> >> WARN_ON(!list_empty(&file->event_list));
> >> diff --git a/drivers/gpu/drm/drm_prime.c
> >> b/drivers/gpu/drm/drm_prime.c
> >> index 1de2cde..c482e9c 100644
> >> --- a/drivers/gpu/drm/drm_prime.c
> >> +++ b/drivers/gpu/drm/drm_prime.c
> >> @@ -335,6 +335,13 @@ int drm_gem_prime_fd_to_handle(struct
> >> drm_device *dev,
> >> ret = drm_prime_add_buf_handle(&file_priv->prime,
> >>dma_buf, *handle);
> >> +
> >> +if (!ret) {
> >> +obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> >> +if (!obj->dummy_page)
> >> +ret = -ENOMEM;
> >> +}
> >> +
> > While the per file case still looks acceptable this is a clear NAK
> > since it will massively increase the memory needed for a prime
> > exported object.
> >
> > I think that this is quite overkill in the first place and for the
> > hot unplug case we can just use the global dummy page as well.
> >
> > Christian.
>
> Global dummy page is good for read access, what do you do on write
> access ? My first approach was indeed to map at first global dummy
> page as read only and mark the vma->vm_flags as !VM_SHARED assuming
> that this would trigger Copy On Write flow in core mm
> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7-rc7%2Fsource%2Fmm%2Fmemory.c%23L3977&data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7C3753451d037544e7495408d816d4c4ee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637284450384586120&sdata=ZpRaQgqA5K4jRfidOiedey0AleeYQ97WNUkGA29ERA0%3D&reserved=0)
>
> on the next page fault to same address triggered by a write access but
> then i realized a new COW page will be allocated for each such mapping
> and this is much more wasteful then having a dedicated page per GEM
> object.
> >>> Yeah, but this is only for a very very small corner cases. What we need
> >>> to prevent is increasing the memory usage during normal operation to
> >>> much.
> >>>
> >>> Using memory during the unplug is completely unproblematic because we
> >>> just released quite a bunch of it by releasing all those system memory
> >>> buffers.
> >>>
> >>> And I'm pretty sure that COWed pages are correctly accounted towards
> >>> the
> >>> used memory of a process.
> >>>
> >>> So I think if that approach works as intended and the COW pages are
> >>> released again on unmapping it would be the perfect solution to the
> >>> problem.
> >>>
> >>> Daniel what do you think?
> >> If COW works, sure sounds reasonable. And if we can make sure we
> >> managed to drop all the system allocations (otherwise suddenly 2x
> >> memory usage, worst case). But I have no idea whether we can
> >> retroshoehorn that into an established vma, you might have fun stuff
> >> like a mkwrite handler there (which I thought is the COW handler
> >> thing, but really no idea).
> >>
> >> If we need to massively change stuff then I think rw dummy page,
> >> allocated on first fault after hotunplug (maybe just make it one per
> >> object, that's simplest) seems like the much safer option. Much less
> >> code that can go wrong.
> >> -Daniel
> >
> >
> > Reg
Re: [PATCH v2 1/8] drm: Add dummy page per device or GEM object
On Sat, Nov 14, 2020 at 10:51 AM Daniel Vetter wrote:
>
> On Sat, Nov 14, 2020 at 9:41 AM Christian König
> wrote:
> >
> > Am 13.11.20 um 21:52 schrieb Andrey Grodzovsky:
> > >
> > > On 6/22/20 1:50 PM, Daniel Vetter wrote:
> > >> On Mon, Jun 22, 2020 at 7:45 PM Christian König
> > >> wrote:
> > >>> Am 22.06.20 um 16:32 schrieb Andrey Grodzovsky:
> > On 6/22/20 9:18 AM, Christian König wrote:
> > > Am 21.06.20 um 08:03 schrieb Andrey Grodzovsky:
> > >> Will be used to reroute CPU mapped BO's page faults once
> > >> device is removed.
> > >>
> > >> Signed-off-by: Andrey Grodzovsky
> > >> ---
> > >>drivers/gpu/drm/drm_file.c | 8
> > >>drivers/gpu/drm/drm_prime.c | 10 ++
> > >>include/drm/drm_file.h | 2 ++
> > >>include/drm/drm_gem.h | 2 ++
> > >>4 files changed, 22 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > >> index c4c704e..67c0770 100644
> > >> --- a/drivers/gpu/drm/drm_file.c
> > >> +++ b/drivers/gpu/drm/drm_file.c
> > >> @@ -188,6 +188,12 @@ struct drm_file *drm_file_alloc(struct
> > >> drm_minor *minor)
> > >>goto out_prime_destroy;
> > >>}
> > >>+file->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> > >> +if (!file->dummy_page) {
> > >> +ret = -ENOMEM;
> > >> +goto out_prime_destroy;
> > >> +}
> > >> +
> > >>return file;
> > >> out_prime_destroy:
> > >> @@ -284,6 +290,8 @@ void drm_file_free(struct drm_file *file)
> > >>if (dev->driver->postclose)
> > >>dev->driver->postclose(dev, file);
> > >>+__free_page(file->dummy_page);
> > >> +
> > >> drm_prime_destroy_file_private(&file->prime);
> > >> WARN_ON(!list_empty(&file->event_list));
> > >> diff --git a/drivers/gpu/drm/drm_prime.c
> > >> b/drivers/gpu/drm/drm_prime.c
> > >> index 1de2cde..c482e9c 100644
> > >> --- a/drivers/gpu/drm/drm_prime.c
> > >> +++ b/drivers/gpu/drm/drm_prime.c
> > >> @@ -335,6 +335,13 @@ int drm_gem_prime_fd_to_handle(struct
> > >> drm_device *dev,
> > >> ret = drm_prime_add_buf_handle(&file_priv->prime,
> > >>dma_buf, *handle);
> > >> +
> > >> +if (!ret) {
> > >> +obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> > >> +if (!obj->dummy_page)
> > >> +ret = -ENOMEM;
> > >> +}
> > >> +
> > > While the per file case still looks acceptable this is a clear NAK
> > > since it will massively increase the memory needed for a prime
> > > exported object.
> > >
> > > I think that this is quite overkill in the first place and for the
> > > hot unplug case we can just use the global dummy page as well.
> > >
> > > Christian.
> >
> > Global dummy page is good for read access, what do you do on write
> > access ? My first approach was indeed to map at first global dummy
> > page as read only and mark the vma->vm_flags as !VM_SHARED assuming
> > that this would trigger Copy On Write flow in core mm
> > (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7-rc7%2Fsource%2Fmm%2Fmemory.c%23L3977&data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7C3753451d037544e7495408d816d4c4ee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637284450384586120&sdata=ZpRaQgqA5K4jRfidOiedey0AleeYQ97WNUkGA29ERA0%3D&reserved=0)
> >
> > on the next page fault to same address triggered by a write access but
> > then i realized a new COW page will be allocated for each such mapping
> > and this is much more wasteful then having a dedicated page per GEM
> > object.
> > >>> Yeah, but this is only for a very very small corner cases. What we need
> > >>> to prevent is increasing the memory usage during normal operation to
> > >>> much.
> > >>>
> > >>> Using memory during the unplug is completely unproblematic because we
> > >>> just released quite a bunch of it by releasing all those system memory
> > >>> buffers.
> > >>>
> > >>> And I'm pretty sure that COWed pages are correctly accounted towards
> > >>> the
> > >>> used memory of a process.
> > >>>
> > >>> So I think if that approach works as intended and the COW pages are
> > >>> released again on unmapping it would be the perfect solution to the
> > >>> problem.
> > >>>
> > >>> Daniel what do you think?
> > >> If COW works, sure sounds reasonable. And if we can make sure we
> > >> managed to drop all the system allocations (otherwise suddenly 2x
> > >> memory usage, worst case). But I have no idea whether we can
> > >> retroshoehorn that into an established vma, you might have fun stuff
> > >> like a mkwrite handler there (which I thought is the COW handler
> > >> thing, but really no idea).
> > >>
> > >> If
Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c
On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote: > > On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote: > > > On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote: > > > > Peilin Ye (5): > > > > console: Delete unused con_font_copy() callback implementations > > > > console: Delete dummy con_font_set() and con_font_default() callback > > > > implementations > > > > Fonts: Add charcount field to font_desc > > > > parisc/sticore: Avoid hard-coding built-in font charcount > > > > fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font charcount > > > > > > Patches all look good to me, if Greg is ok with me applying the entire > > > pile to drm-misc-next I'll do that next week. > > On Fri, Nov 13, 2020 at 11:47:51PM +0100, Greg Kroah-Hartman wrote: > > Reviewed-by: Greg Kroah-Hartman > > Thanks for reviewing! Questions about the last patch [5/5] though, it > depends on the following assumption: > > """ > For each console `idx`, `vc_cons[idx].d->vc_font.data` and > `fb_display[idx].fontdata` always point to the same buffer. > """ > > Is this true? I think it is true by grepping for `fontdata`. I also > noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata` > interchangeably, see fbcon_get_requirement(): > > vc = vc_cons[fg_console].d; > [...] > p = &fb_display[fg_console]; > caps->x = 1 << (vc->vc_font.width - 1); > ^^^ > caps->y = 1 << (vc->vc_font.height - 1); > ^^^ > caps->len = (p->userfont) ? > FNTCHARCNT(p->fontdata) : 256; > ^^^ > > If it is true, then what is the point of using `fontdata` in `struct > fbcon_display`? Just for the `userfont` flag? Should we delete > `fontdata`, when we no longer need the `userfont` flag? Yes, after a quick look, I think your analysis here is correct. So if you can delete that, it would be nice if possible. > In this sense I think [5/5] needs more testing. Do we have test files > for fbcon, or should I try to write some tests from scratch? I don't know of any direct tests, I usually just booted into that mode and saw if everything looked like it did before. There must be some tool that you can use to change the current font, as it seems to happen at boot time on some distros. I can't remember what it's called at the moment... thanks, greg k-h ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c
On Sat, Nov 14, 2020 at 01:18:06PM +0100, Greg Kroah-Hartman wrote: > On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote: > > > On Fri, Nov 13, 2020 at 10:16:33PM +0100, Daniel Vetter wrote: > > > > On Thu, Nov 12, 2020 at 07:02:21AM -0500, Peilin Ye wrote: > > > > > Peilin Ye (5): > > > > > console: Delete unused con_font_copy() callback implementations > > > > > console: Delete dummy con_font_set() and con_font_default() > > > > > callback implementations > > > > > Fonts: Add charcount field to font_desc > > > > > parisc/sticore: Avoid hard-coding built-in font charcount > > > > > fbcon: Avoid using FNTCHARCNT() and hard-coded built-in font > > > > > charcount > > > > > > > > Patches all look good to me, if Greg is ok with me applying the entire > > > > pile to drm-misc-next I'll do that next week. > > > > On Fri, Nov 13, 2020 at 11:47:51PM +0100, Greg Kroah-Hartman wrote: > > > Reviewed-by: Greg Kroah-Hartman > > > > Thanks for reviewing! Questions about the last patch [5/5] though, it > > depends on the following assumption: > > > > """ > > For each console `idx`, `vc_cons[idx].d->vc_font.data` and > > `fb_display[idx].fontdata` always point to the same buffer. > > """ > > > > Is this true? I think it is true by grepping for `fontdata`. I also > > noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata` > > interchangeably, see fbcon_get_requirement(): > > > > vc = vc_cons[fg_console].d; > > [...] > > p = &fb_display[fg_console]; > > caps->x = 1 << (vc->vc_font.width - 1); > > ^^^ > > caps->y = 1 << (vc->vc_font.height - 1); > > ^^^ > > caps->len = (p->userfont) ? > > FNTCHARCNT(p->fontdata) : 256; > >^^^ > > > > If it is true, then what is the point of using `fontdata` in `struct > > fbcon_display`? Just for the `userfont` flag? Should we delete > > `fontdata`, when we no longer need the `userfont` flag? > > Yes, after a quick look, I think your analysis here is correct. So if > you can delete that, it would be nice if possible. > > > In this sense I think [5/5] needs more testing. Do we have test files > > for fbcon, or should I try to write some tests from scratch? > > I don't know of any direct tests, I usually just booted into that mode > and saw if everything looked like it did before. There must be some > tool that you can use to change the current font, as it seems to happen > at boot time on some distros. I can't remember what it's called at the > moment... Ah, here's a hint: https://wiki.archlinux.org/index.php/Linux_console#Fonts The setfont tool should help you out here. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/5] console: Miscellaneous clean-ups, do not use FNTCHARCNT() in fbcon.c
On Sat, Nov 14, 2020 at 01:22:22PM +0100, Greg Kroah-Hartman wrote: > On Sat, Nov 14, 2020 at 01:18:06PM +0100, Greg Kroah-Hartman wrote: > > On Sat, Nov 14, 2020 at 03:10:21AM -0500, Peilin Ye wrote: > > > Thanks for reviewing! Questions about the last patch [5/5] though, it > > > depends on the following assumption: > > > > > > """ > > > For each console `idx`, `vc_cons[idx].d->vc_font.data` and > > > `fb_display[idx].fontdata` always point to the same buffer. > > > """ > > > > > > Is this true? I think it is true by grepping for `fontdata`. I also > > > noticed that fbcon.c is using `vc->vc_font.data` and `p->fontdata` > > > interchangeably, see fbcon_get_requirement(): > > > > > > vc = vc_cons[fg_console].d; > > > [...] > > > p = &fb_display[fg_console]; > > > caps->x = 1 << (vc->vc_font.width - 1); > > > ^^^ > > > caps->y = 1 << (vc->vc_font.height - 1); > > > ^^^ > > > caps->len = (p->userfont) ? > > > FNTCHARCNT(p->fontdata) : 256; > > > ^^^ > > > > > > If it is true, then what is the point of using `fontdata` in `struct > > > fbcon_display`? Just for the `userfont` flag? Should we delete > > > `fontdata`, when we no longer need the `userfont` flag? > > > > Yes, after a quick look, I think your analysis here is correct. So if > > you can delete that, it would be nice if possible. I see, at the moment we still need `userfont` for refcount handling, I will try to delete both `fontdata` and `userfont` after refcount is taken care of. > > > In this sense I think [5/5] needs more testing. Do we have test files > > > for fbcon, or should I try to write some tests from scratch? > > > > I don't know of any direct tests, I usually just booted into that mode > > and saw if everything looked like it did before. There must be some > > tool that you can use to change the current font, as it seems to happen > > at boot time on some distros. I can't remember what it's called at the > > moment... > > Ah, here's a hint: > https://wiki.archlinux.org/index.php/Linux_console#Fonts > > The setfont tool should help you out here. Oh, I didn't know about this one. I'll go experimenting with it, thank you! Peilin Ye ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 01/26] memory: tegra: Correct stub of devm_tegra_memory_controller_get()
On Wed, Nov 11, 2020 at 04:14:31AM +0300, Dmitry Osipenko wrote: > Correct typo in a stub of devm_tegra_memory_controller_get() to fix a > non-ARM kernel compile-testing. > > Reported-by: Stephen Rothwell > Signed-off-by: Dmitry Osipenko > --- > include/soc/tegra/mc.h | 2 +- Thanks, applied. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 02/26] memory: tegra20-emc: Use dev_pm_opp_set_clkname()
On Wed, Nov 11, 2020 at 04:14:32AM +0300, Dmitry Osipenko wrote: > The dev_pm_opp_get_opp_table() shouldn't be used by drivers, use > dev_pm_opp_set_clkname() instead. > > Suggested-by: Viresh Kumar > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/tegra/tegra20-emc.c | 30 +++--- > 1 file changed, 19 insertions(+), 11 deletions(-) Thanks, applied. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 03/26] memory: tegra20-emc: Factor out clk initialization
On Wed, Nov 11, 2020 at 04:14:33AM +0300, Dmitry Osipenko wrote: > Factor out clk initialization and make it resource-managed. This makes > easier to follow code and will help to make further changes cleaner. > > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/tegra/tegra20-emc.c | 70 -- > 1 file changed, 47 insertions(+), 23 deletions(-) Thanks, applied. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 04/26] memory: tegra20-emc: Add devfreq support
On Wed, Nov 11, 2020 at 04:14:34AM +0300, Dmitry Osipenko wrote: > Add devfreq support to the Tegra20 EMC driver. Memory utilization > statistics will be periodically polled from the memory controller and > appropriate minimum clock rate will be selected by the devfreq governor. > > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/tegra/Kconfig | 3 +- > drivers/memory/tegra/tegra20-emc.c | 90 ++ > 2 files changed, 92 insertions(+), 1 deletion(-) Thanks, applied. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 05/26] memory: tegra20-emc: Remove IRQ number from error message
On Wed, Nov 11, 2020 at 04:14:35AM +0300, Dmitry Osipenko wrote: > Remove IRQ number from error message since it doesn't add any useful > information, especially because this number is virtual. > > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/tegra/tegra20-emc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 06/26] memory: tegra30: Add FIFO sizes to memory clients
On Wed, Nov 11, 2020 at 04:14:36AM +0300, Dmitry Osipenko wrote: > The latency allowness is calculated based on buffering capabilities of > memory clients. Add FIFO sizes to the Tegra30 memory clients. > > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/tegra/tegra30.c | 66 ++ > 1 file changed, 66 insertions(+) Thanks, applied. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 07/26] memory: tegra30-emc: Make driver modular
On Wed, Nov 11, 2020 at 04:14:37AM +0300, Dmitry Osipenko wrote: > Add modularization support to the Tegra30 EMC driver, which now can be > compiled as a loadable kernel module. > > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/tegra/Kconfig | 2 +- > drivers/memory/tegra/mc.c | 3 +++ > drivers/memory/tegra/tegra30-emc.c | 17 - Thanks, applied. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 08/26] memory: tegra30-emc: Continue probing if timings are missing in device-tree
On Wed, Nov 11, 2020 at 04:14:38AM +0300, Dmitry Osipenko wrote: > EMC driver will become mandatory after turning it into interconnect > provider because interconnect users, like display controller driver, will > fail to probe using newer device-trees that have interconnect properties. > Thus make EMC driver to probe even if timings are missing in device-tree. > > Signed-off-by: Dmitry Osipenko > --- > drivers/memory/tegra/tegra30-emc.c | 29 +++-- > 1 file changed, 15 insertions(+), 14 deletions(-) Thanks, applied 1-8. For the rest I see discussion on going, so I guess there will be a v9. Best regards, Krzysztof ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 210201] New: [amdpgu] crash when playing after suspend/resume
https://bugzilla.kernel.org/show_bug.cgi?id=210201 Bug ID: 210201 Summary: [amdpgu] crash when playing after suspend/resume Product: Drivers Version: 2.5 Kernel Version: 5.9.8, 5.6.19, 5.8.18 Hardware: x86-64 OS: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: Video(DRI - non Intel) Assignee: drivers_video-...@kernel-bugs.osdl.org Reporter: arturbac...@gmail.com Regression: No Created attachment 293669 --> https://bugzilla.kernel.org/attachment.cgi?id=293669&action=edit Full dmesg after video crash When i play vulkan games, like Kerbal Space Program, ReadDeadRedemption2(via Proton) after i return from suspend, after runing them after 30min - 1h graphics driver crashes. OS: Gentoo Kernel: x86_64 Linux 5.9.8 Resolution: 7680x2160 (2 monitors attached 4K free sync) DE: KDE 5.75.0 / Plasma 5.20.3 WM: KWin GTK Theme: Adwaita [GTK2/3] CPU: AMD Ryzen 9 3900X 12-Core @ 24x 3.8GHz GPU: AMD Radeon RX 5700 XT (NAVI10, DRM 3.39.0, 5.9.8, LLVM 10.0.1) Mesa 20.2.2 RAM: 32038MiB Full dmesg attached. [104307.850190] amdgpu :0f:00.0: amdgpu: ring gfx_0.0.0 uses VM inv eng 0 on hub 0 [104307.850192] amdgpu :0f:00.0: amdgpu: ring comp_1.0.0 uses VM inv eng 1 on hub 0 [104307.850194] amdgpu :0f:00.0: amdgpu: ring comp_1.1.0 uses VM inv eng 4 on hub 0 [104307.850195] amdgpu :0f:00.0: amdgpu: ring comp_1.2.0 uses VM inv eng 5 on hub 0 [104307.850196] amdgpu :0f:00.0: amdgpu: ring comp_1.3.0 uses VM inv eng 6 on hub 0 [104307.850198] amdgpu :0f:00.0: amdgpu: ring comp_1.0.1 uses VM inv eng 7 on hub 0 [104307.850199] amdgpu :0f:00.0: amdgpu: ring comp_1.1.1 uses VM inv eng 8 on hub 0 [104307.850201] amdgpu :0f:00.0: amdgpu: ring comp_1.2.1 uses VM inv eng 9 on hub 0 [104307.850202] amdgpu :0f:00.0: amdgpu: ring comp_1.3.1 uses VM inv eng 10 on hub 0 [104307.850203] amdgpu :0f:00.0: amdgpu: ring kiq_2.1.0 uses VM inv eng 11 on hub 0 [104307.850205] amdgpu :0f:00.0: amdgpu: ring sdma0 uses VM inv eng 12 on hub 0 [104307.850206] amdgpu :0f:00.0: amdgpu: ring sdma1 uses VM inv eng 13 on hub 0 [104307.850208] amdgpu :0f:00.0: amdgpu: ring vcn_dec uses VM inv eng 0 on hub 1 [104307.850209] amdgpu :0f:00.0: amdgpu: ring vcn_enc0 uses VM inv eng 1 on hub 1 [104307.850210] amdgpu :0f:00.0: amdgpu: ring vcn_enc1 uses VM inv eng 4 on hub 1 [104307.850212] amdgpu :0f:00.0: amdgpu: ring jpeg_dec uses VM inv eng 5 on hub 1 [104307.852128] [drm] recover vram bo from shadow start [104307.872340] [drm] recover vram bo from shadow done [104307.872342] [drm] Skip scheduling IBs! [104307.872343] [drm] Skip scheduling IBs! [104307.872357] [drm] Skip scheduling IBs! [104307.872362] amdgpu :0f:00.0: amdgpu: GPU reset(2) succeeded! [104307.872373] [drm] Skip scheduling IBs! [repeated many times...] [104307.872440] [drm] Skip scheduling IBs! [104314.769174] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-16) [104314.795600] [drm:amdgpu_gem_va_ioctl [amdgpu]] *ERROR* Couldn't update BO_VA (-16) [104314.847946] amdgpu :0f:00.0: amdgpu: failed to clear page tables on GEM object close (-16) [repeated many times...] [104315.300254] amdgpu :0f:00.0: amdgpu: failed to clear page tables on GEM object close (-16) [104325.487235] GpuWatchdog[731778]: segfault at 0 ip 7f9be2bf92dd sp 7f9bd77ed670 error 6 in libcef.so[7f9bdee73000+69a4000] [104325.488266] Code: 00 79 09 48 8b 7d a0 e8 21 80 c1 02 41 8b 85 00 01 00 00 85 c0 0f 84 ab 00 00 00 49 8b 45 00 4c 89 ef be 01 00 00 00 ff 50 58 04 25 00 00 00 00 37 13 00 00 c6 05 c1 a5 37 03 01 80 bd 7f ff [104335.590624] GpuWatchdog[731809]: segfault at 0 ip 7f494f6142dd sp 7f4944208670 error 6 in libcef.so[7f494b88e000+69a4000] [104335.590631] Code: 00 79 09 48 8b 7d a0 e8 21 80 c1 02 41 8b 85 00 01 00 00 85 c0 0f 84 ab 00 00 00 49 8b 45 00 4c 89 ef be 01 00 00 00 ff 50 58 04 25 00 00 00 00 37 13 00 00 c6 05 c1 a5 37 03 01 80 bd 7f ff [104345.690401] GpuWatchdog[731833]: segfault at 0 ip 7fcbb4c722dd sp 7fcba9866670 error 6 in libcef.so[7fcbb0eec000+69a4000] [104345.692176] Code: 00 79 09 48 8b 7d a0 e8 21 80 c1 02 41 8b 85 00 01 00 00 85 c0 0f 84 ab 00 00 00 49 8b 45 00 4c 89 ef be 01 00 00 00 ff 50 58 04 25 00 00 00 00 37 13 00 00 c6 05 c1 a5 37 03 01 80 bd 7f ff -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 208205] [amdgpu] System hang / freeze
https://bugzilla.kernel.org/show_bug.cgi?id=208205 --- Comment #6 from Andrea Borgia (and...@borgia.bo.it) --- Running linux-image-5.9.0-1-amd64 for quite some time now and it looks stable. AFAIK this was the fix: https://lkml.org/lkml/2020/7/27/64 -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v2 4/5] drm/msm: add DRM_MSM_GEM_SYNC_CACHE for non-coherent cache maintenance
On Sat, Nov 14, 2020 at 8:24 AM Christoph Hellwig wrote:
>
> On Sat, Nov 14, 2020 at 10:17:12AM -0500, Jonathan Marek wrote:
> > +void msm_gem_sync_cache(struct drm_gem_object *obj, uint32_t flags,
> > + size_t range_start, size_t range_end)
> > +{
> > + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > + struct device *dev = msm_obj->base.dev->dev;
> > +
> > + /* exit early if get_pages() hasn't been called yet */
> > + if (!msm_obj->pages)
> > + return;
> > +
> > + /* TODO: sync only the specified range */
> > +
> > + if (flags & MSM_GEM_SYNC_FOR_DEVICE) {
> > + dma_sync_sg_for_device(dev, msm_obj->sgt->sgl,
> > + msm_obj->sgt->nents, DMA_TO_DEVICE);
> > + }
> > +
> > + if (flags & MSM_GEM_SYNC_FOR_CPU) {
> > + dma_sync_sg_for_cpu(dev, msm_obj->sgt->sgl,
> > + msm_obj->sgt->nents, DMA_FROM_DEVICE);
> > + }
>
> Splitting this helper from the only caller is rather strange, epecially
> with the two unused arguments. And I think the way this is specified
> to take a range, but ignoring it is actively dangerous. User space will
> rely on it syncing everything sooner or later and then you are stuck.
> So just define a sync all primitive for now, and if you really need a
> range sync and have actually implemented it add a new ioctl for that.
We do already have a split of ioctl "layer" which enforces valid ioctl
params, etc, and gem (or other) module code which is called by the
ioctl func. So I think it is fine to keep this split here. (Also, I
think at some point there will be a uring type of ioctl alternative
which would re-use the same gem func.)
But I do agree that the range should be respected or added later..
drm_ioctl() dispatch is well prepared for extending ioctls.
And I assume there should be some validation that the range is aligned
to cache-line? Or can we flush a partial cache line?
BR,
-R
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/tegra: output: Do not put OF node twice
Hi Thierry,
On Fri, Nov 13, 2020 at 09:41:57PM +0100, Thierry Reding wrote:
> From: Thierry Reding
>
> The original patch for commit 3d2e7aec7013 ("drm/tegra: output: Don't
> leak OF node on error") contained this hunk, but it was accidentally
> dropped during conflict resolution. This causes use-after-free errors
> on devices that use an I2C controller for HDMI DDC/CI on Tegra210 and
> later.
>
> Fixes: 3d2e7aec7013 ("drm/tegra: output: Don't leak OF node on error")
> Signed-off-by: Thierry Reding
Two times of_node_put() of the same pointer looks bad.
Reviewed-by: Sam Ravnborg
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 12/40] drm/pl111/pl111_display: Make local function static
Hi Lee,
On Fri, Nov 13, 2020 at 01:49:10PM +, Lee Jones wrote:
> Fixes the following W=1 kernel build warning(s):
>
> drivers/gpu/drm/pl111/pl111_display.c:356:6: warning: no previous prototype
> for ‘pl111_display_disable’ [-Wmissing-prototypes]
>
> Cc: Eric Anholt
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Lee Jones
Eric's was not copied on this or the other pl111 patch.
Added Eric so he can be aware of this fix.
Sam
> ---
> drivers/gpu/drm/pl111/pl111_display.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c
> b/drivers/gpu/drm/pl111/pl111_display.c
> index b3e8697cafcf1..69c02e7c82b7e 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -353,7 +353,7 @@ static void pl111_display_enable(struct
> drm_simple_display_pipe *pipe,
> drm_crtc_vblank_on(crtc);
> }
>
> -void pl111_display_disable(struct drm_simple_display_pipe *pipe)
> +static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
> {
> struct drm_crtc *crtc = &pipe->crtc;
> struct drm_device *drm = crtc->dev;
> --
> 2.25.1
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/40] drm/panel/panel-tpo-tpg110: Correct misnaming and supply missing param description
Hi Lee, On Fri, Nov 13, 2020 at 01:49:11PM +, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/panel/panel-tpo-tpg110.c:94: warning: Function parameter or > member 'panel_mode' not described in 'tpg110' > drivers/gpu/drm/panel/panel-tpo-tpg110.c:372: warning: Function parameter or > member 'connector' not described in 'tpg110_get_modes' > > Cc: Linus Walleij > Cc: Thierry Reding > Cc: Sam Ravnborg > Cc: David Airlie > Cc: Daniel Vetter > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Lee Jones Thanks, applied to drm-misc-next. Only one warning left in drm/panel/, which I will submit a patch for in a minute. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/msm/shrinker: We can vmap shrink active_list too
From: Rob Clark
Just because a obj is active, if the vmap_count is zero, we can still
tear down the vmap.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem_shrinker.c | 47 +++---
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index 6f4b1355725f..9d51c1eb808d 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -6,6 +6,7 @@
#include "msm_drv.h"
#include "msm_gem.h"
+#include "msm_gpu.h"
#include "msm_gpu_trace.h"
static unsigned long
@@ -61,17 +62,19 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
shrink_control *sc)
return freed;
}
-static int
-msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void
*ptr)
+/* since we don't know any better, lets bail after a few
+ * and if necessary the shrinker will be invoked again.
+ * Seems better than unmapping *everything*
+ */
+static const int vmap_shrink_limit = 15;
+
+static unsigned
+vmap_shrink(struct list_head *mm_list)
{
- struct msm_drm_private *priv =
- container_of(nb, struct msm_drm_private, vmap_notifier);
struct msm_gem_object *msm_obj;
unsigned unmapped = 0;
- mutex_lock(&priv->mm_lock);
-
- list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
+ list_for_each_entry(msm_obj, mm_list, mm_list) {
if (!msm_gem_trylock(&msm_obj->base))
continue;
if (is_vunmapable(msm_obj)) {
@@ -80,11 +83,31 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned
long event, void *ptr)
}
msm_gem_unlock(&msm_obj->base);
- /* since we don't know any better, lets bail after a few
-* and if necessary the shrinker will be invoked again.
-* Seems better than unmapping *everything*
-*/
- if (++unmapped >= 15)
+ if (++unmapped >= vmap_shrink_limit)
+ break;
+ }
+
+ return unmapped;
+}
+
+static int
+msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void
*ptr)
+{
+ struct msm_drm_private *priv =
+ container_of(nb, struct msm_drm_private, vmap_notifier);
+ struct list_head *mm_lists[] = {
+ &priv->inactive_list,
+ priv->gpu ? &priv->gpu->active_list : NULL,
+ NULL,
+ };
+ unsigned idx, unmapped = 0;
+
+ mutex_lock(&priv->mm_lock);
+
+ for (idx = 0; mm_lists[idx]; idx++) {
+ unmapped += vmap_shrink(mm_lists[idx]);
+
+ if (unmapped >= vmap_shrink_limit)
break;
}
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] drm/msm: Shrinker fixes and opts
From: Rob Clark The last patch is the main thing, motivated by some cases where we would spend a lot of time in msm_gem_shrinker_count(). First two are fixes I noticed along the way. Rob Clark (3): drm/msm: Protect obj->active_count under obj lock drm/msm/shrinker: We can vmap shrink active_list too drm/msm/shrinker: Only iterate dontneed objs drivers/gpu/drm/msm/msm_debugfs.c | 3 +- drivers/gpu/drm/msm/msm_drv.c | 3 +- drivers/gpu/drm/msm/msm_drv.h | 8 ++-- drivers/gpu/drm/msm/msm_gem.c | 45 -- drivers/gpu/drm/msm/msm_gem.h | 5 ++- drivers/gpu/drm/msm/msm_gem_shrinker.c | 52 +++--- drivers/gpu/drm/msm/msm_gpu.c | 10 +++-- 7 files changed, 89 insertions(+), 37 deletions(-) -- 2.28.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs
From: Rob Clark
In situations where the GPU is mostly idle, all or nearly all buffer
objects will be in the inactive list. But if the system is under memory
pressure (from something other than GPU), we could still get a lot of
shrinker calls. Which results in traversing a list of thousands of objs
and in the end finding nothing to shrink. Which isn't so efficient.
Instead split the inactive_list into two lists, one inactive objs which
are shrinkable, and a second one for those that are not. This way we
can avoid traversing objs which we know are not shrinker candidates.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_debugfs.c | 3 ++-
drivers/gpu/drm/msm/msm_drv.c | 3 ++-
drivers/gpu/drm/msm/msm_drv.h | 8 +++---
drivers/gpu/drm/msm/msm_gem.c | 34 --
drivers/gpu/drm/msm/msm_gem_shrinker.c | 7 +++---
5 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_debugfs.c
b/drivers/gpu/drm/msm/msm_debugfs.c
index 64afbed89821..85ad0babc326 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -124,7 +124,8 @@ static int msm_gem_show(struct drm_device *dev, struct
seq_file *m)
}
seq_printf(m, "Inactive Objects:\n");
- msm_gem_describe_objects(&priv->inactive_list, m);
+ msm_gem_describe_objects(&priv->inactive_dontneed, m);
+ msm_gem_describe_objects(&priv->inactive_willneed, m);
mutex_unlock(&priv->mm_lock);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4d808769e6ed..39a54f364aa8 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -465,7 +465,8 @@ static int msm_drm_init(struct device *dev, struct
drm_driver *drv)
priv->wq = alloc_ordered_workqueue("msm", 0);
- INIT_LIST_HEAD(&priv->inactive_list);
+ INIT_LIST_HEAD(&priv->inactive_willneed);
+ INIT_LIST_HEAD(&priv->inactive_dontneed);
mutex_init(&priv->mm_lock);
/* Teach lockdep about lock ordering wrt. shrinker: */
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index f869ed67b5da..ed18c5bed10f 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -175,8 +175,9 @@ struct msm_drm_private {
struct msm_perf_state *perf;
/*
-* List of inactive GEM objects. Every bo is either in the
inactive_list
-* or gpu->active_list (for the gpu it is active on[1])
+* Lists of inactive GEM objects. Every bo is either in one of the
+* inactive lists (depending on whether or not it is shrinkable) or
+* gpu->active_list (for the gpu it is active on[1])
*
* These lists are protected by mm_lock. If struct_mutex is involved,
it
* should be aquired prior to mm_lock. One should *not* hold mm_lock in
@@ -185,7 +186,8 @@ struct msm_drm_private {
* [1] if someone ever added support for the old 2d cores, there could
be
* more than one gpu object
*/
- struct list_head inactive_list;
+ struct list_head inactive_willneed; /* inactive + !shrinkable */
+ struct list_head inactive_dontneed; /* inactive + shrinkable */
struct mutex mm_lock;
struct workqueue_struct *wq;
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 2795288b0a95..de8d2cfada24 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -17,6 +17,7 @@
#include "msm_gpu.h"
#include "msm_mmu.h"
+static void update_inactive(struct msm_gem_object *msm_obj);
static dma_addr_t physaddr(struct drm_gem_object *obj)
{
@@ -678,6 +679,12 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned
madv)
madv = msm_obj->madv;
+ /* If the obj is inactive, we might need to move it
+* between inactive lists
+*/
+ if (msm_obj->active_count == 0)
+ update_inactive(msm_obj);
+
msm_gem_unlock(obj);
return (madv != __MSM_MADV_PURGED);
@@ -781,19 +788,31 @@ void msm_gem_active_get(struct drm_gem_object *obj,
struct msm_gpu *gpu)
void msm_gem_active_put(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- struct msm_drm_private *priv = obj->dev->dev_private;
might_sleep();
WARN_ON(!msm_gem_is_locked(obj));
if (--msm_obj->active_count == 0) {
- mutex_lock(&priv->mm_lock);
- list_del_init(&msm_obj->mm_list);
- list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
- mutex_unlock(&priv->mm_lock);
+ update_inactive(msm_obj);
}
}
+static void update_inactive(struct msm_gem_object *msm_obj)
+{
+ struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
+
+ mutex_lock(&priv->mm_lock);
+ WARN_ON(msm_obj->active_count != 0);
+
+ list_
[PATCH 1/3] drm/msm: Protect obj->active_count under obj lock
From: Rob Clark
Previously we only held obj lock in the _active_get() path, and relied
on atomic_dec_return() to not be racy in the _active_put() path where
obj lock was not held.
But this is a false sense of security. Unlike obj lifetime refcnt,
where you do not expect to *increase* the refcnt after the last put
(which would mean that something has gone horribly wrong with the
object liveness reference counting), the active_count can increase
again from zero. Racing _active_put()s and _active_get()s could leave
the obj on the wrong mm list.
But in the retire path, immediately after the _active_put(), the
_unpin_iova() would acquire obj lock. So just move the locking earlier
and rely on that to protect obj->active_count.
Fixes: c5c1643cef7a ("drm/msm: Drop struct_mutex from the retire path")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 11 ++-
drivers/gpu/drm/msm/msm_gem.h | 5 +++--
drivers/gpu/drm/msm/msm_gpu.c | 10 ++
3 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index a9a834bb7794..2795288b0a95 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -770,7 +770,7 @@ void msm_gem_active_get(struct drm_gem_object *obj, struct
msm_gpu *gpu)
WARN_ON(!msm_gem_is_locked(obj));
WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
- if (!atomic_fetch_inc(&msm_obj->active_count)) {
+ if (msm_obj->active_count++ == 0) {
mutex_lock(&priv->mm_lock);
list_del_init(&msm_obj->mm_list);
list_add_tail(&msm_obj->mm_list, &gpu->active_list);
@@ -784,8 +784,9 @@ void msm_gem_active_put(struct drm_gem_object *obj)
struct msm_drm_private *priv = obj->dev->dev_private;
might_sleep();
+ WARN_ON(!msm_gem_is_locked(obj));
- if (!atomic_dec_return(&msm_obj->active_count)) {
+ if (--msm_obj->active_count == 0) {
mutex_lock(&priv->mm_lock);
list_del_init(&msm_obj->mm_list);
list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
@@ -936,15 +937,15 @@ void msm_gem_free_object(struct drm_gem_object *obj)
struct drm_device *dev = obj->dev;
struct msm_drm_private *priv = dev->dev_private;
- /* object should not be on active list: */
- WARN_ON(is_active(msm_obj));
-
mutex_lock(&priv->mm_lock);
list_del(&msm_obj->mm_list);
mutex_unlock(&priv->mm_lock);
msm_gem_lock(obj);
+ /* object should not be on active list: */
+ WARN_ON(is_active(msm_obj));
+
put_iova(obj);
if (obj->import_attach) {
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index d79e7019cc88..3355a48a023b 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -87,7 +87,7 @@ struct msm_gem_object {
char name[32]; /* Identifier to print for the debugfs files */
- atomic_t active_count;
+ int active_count;
};
#define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
@@ -185,7 +185,8 @@ msm_gem_is_locked(struct drm_gem_object *obj)
static inline bool is_active(struct msm_gem_object *msm_obj)
{
- return atomic_read(&msm_obj->active_count);
+ WARN_ON(!msm_gem_is_locked(&msm_obj->base));
+ return msm_obj->active_count;
}
static inline bool is_purgeable(struct msm_gem_object *msm_obj)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index b597213a1890..04f7ab4d63ae 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -717,11 +717,13 @@ static void retire_submit(struct msm_gpu *gpu, struct
msm_ringbuffer *ring,
stats->alwayson_start, stats->alwayson_end);
for (i = 0; i < submit->nr_bos; i++) {
- struct msm_gem_object *msm_obj = submit->bos[i].obj;
+ struct drm_gem_object *obj = &submit->bos[i].obj->base;
- msm_gem_active_put(&msm_obj->base);
- msm_gem_unpin_iova(&msm_obj->base, submit->aspace);
- drm_gem_object_put(&msm_obj->base);
+ msm_gem_lock(obj);
+ msm_gem_active_put(obj);
+ msm_gem_unpin_iova_locked(obj, submit->aspace);
+ msm_gem_unlock(obj);
+ drm_gem_object_put(obj);
}
pm_runtime_mark_last_busy(&gpu->pdev->dev);
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 1/1] drm/panel: samsumg-ld9040: fix spi id_table
A W=1 build resulted in the following warning:
panel-samsung-ld9040.c:377:35: warning: ‘ld9040_ids’ defined but not used
The root cause was a missing assignment to spi_driver.id_table.
Fixes: 6915db346039 ("drm/panel: ld9040: add MODULE_DEVICE_TABLE with SPI IDs")
Cc: Marek Szyprowski
Cc: Lee Jones
Cc: Sam Ravnborg
Cc: Thierry Reding
Cc: dri-devel@lists.freedesktop.org
Cc: # v5.7+
Signed-off-by: Sam Ravnborg
---
drivers/gpu/drm/panel/panel-samsung-ld9040.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
index f484147fc3a6..c4b388850a13 100644
--- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
+++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
@@ -383,6 +383,7 @@ MODULE_DEVICE_TABLE(spi, ld9040_ids);
static struct spi_driver ld9040_driver = {
.probe = ld9040_probe,
.remove = ld9040_remove,
+ .id_table = ld9040_ids,
.driver = {
.name = "panel-samsung-ld9040",
.of_match_table = ld9040_of_match,
--
2.27.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 1/1] drm/panel: samsumg-ld9040: fix spi id_table
Hi myself. s/samsumg/samsung/ in the subject. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v2 4/5] drm/msm: add DRM_MSM_GEM_SYNC_CACHE for non-coherent cache maintenance
On Sat, Nov 14, 2020 at 10:58 AM Jonathan Marek wrote:
>
> On 11/14/20 1:46 PM, Rob Clark wrote:
> > On Sat, Nov 14, 2020 at 8:24 AM Christoph Hellwig wrote:
> >>
> >> On Sat, Nov 14, 2020 at 10:17:12AM -0500, Jonathan Marek wrote:
> >>> +void msm_gem_sync_cache(struct drm_gem_object *obj, uint32_t flags,
> >>> + size_t range_start, size_t range_end)
> >>> +{
> >>> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> >>> + struct device *dev = msm_obj->base.dev->dev;
> >>> +
> >>> + /* exit early if get_pages() hasn't been called yet */
> >>> + if (!msm_obj->pages)
> >>> + return;
> >>> +
> >>> + /* TODO: sync only the specified range */
> >>> +
> >>> + if (flags & MSM_GEM_SYNC_FOR_DEVICE) {
> >>> + dma_sync_sg_for_device(dev, msm_obj->sgt->sgl,
> >>> + msm_obj->sgt->nents, DMA_TO_DEVICE);
> >>> + }
> >>> +
> >>> + if (flags & MSM_GEM_SYNC_FOR_CPU) {
> >>> + dma_sync_sg_for_cpu(dev, msm_obj->sgt->sgl,
> >>> + msm_obj->sgt->nents, DMA_FROM_DEVICE);
> >>> + }
> >>
> >> Splitting this helper from the only caller is rather strange, epecially
> >> with the two unused arguments. And I think the way this is specified
> >> to take a range, but ignoring it is actively dangerous. User space will
> >> rely on it syncing everything sooner or later and then you are stuck.
> >> So just define a sync all primitive for now, and if you really need a
> >> range sync and have actually implemented it add a new ioctl for that.
> >
> > We do already have a split of ioctl "layer" which enforces valid ioctl
> > params, etc, and gem (or other) module code which is called by the
> > ioctl func. So I think it is fine to keep this split here. (Also, I
> > think at some point there will be a uring type of ioctl alternative
> > which would re-use the same gem func.)
> >
> > But I do agree that the range should be respected or added later..
> > drm_ioctl() dispatch is well prepared for extending ioctls.
> >
> > And I assume there should be some validation that the range is aligned
> > to cache-line? Or can we flush a partial cache line?
> >
>
> The range is intended to be "sync at least this range", so that
> userspace doesn't have to worry about details like that.
>
I don't think userspace can *not* worry about details like that.
Consider a case where the cpu and gpu are simultaneously accessing
different parts of a buffer (for ex, sub-allocation). There needs to
be cache-line separation between the two.
BR,
-R
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] drm/panel/samsung-sofef00: Add panel for OnePlus 6/T devices
Hi Caleb, On Thu, Nov 12, 2020 at 04:21:13PM +, Caleb Connolly wrote: > The OnePlus 6/T devices use different panels however they are > functionally identical with the only differences being the resolution. > The panels also don't seem to be used by any other devices, just combine > them into one driver. > > The panels are: samsung,sofef00 > and samsung,s6e3fc2x01 > > Signed-off-by: Caleb Connolly Thanks, applied to drm-misc-next. Fixed a few checkpatch --strict warnings when applying. Please check with --strict in the future. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/5] dt-bindings: panel-simple-dsi: add samsung panels for OnePlus 6/T
Hi Caleb, On Thu, Nov 12, 2020 at 04:21:30PM +, Caleb Connolly wrote: > Add compatibles for the samsung,sofef00 and samsung,s6e3fc2x01 panels > used in the OnePlus 6 & 6T respectively. > > Signed-off-by: Caleb Connolly Thansk, applied to drm-misc-next. Fixed so entries are sorted alphabetically when applying. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] dt-bindings: display: mcde: Convert to YAML schema
On Thu, Nov 12, 2020 at 01:19:34PM +0100, Linus Walleij wrote: > On Wed, Nov 11, 2020 at 9:59 PM Sam Ravnborg wrote: > > On Wed, Nov 11, 2020 at 02:07:54PM +0100, Linus Walleij wrote: > > > > -- clocks: an array of the MCDE clocks in this strict order: > > > - MCDECLK (main MCDE clock), LCDCLK (LCD clock), PLLDSI > > > - (HDMI clock), DSI0ESCLK (DSI0 energy save clock), > > > > > - DSI1ESCLK (DSI1 energy save clock), DSI2ESCLK (DSI2 energy > > > - save clock) > > > > I did not find these two clocks in the binding below. > > The old bindings are wrong. These clocks belong on the DSI > host adapters, so they are in this part of the binding: Please include this info in the changelog to avoid confusing others. > > + clocks: > +description: phandles to the high speed and low power (energy > save) clocks > + the high speed clock is not present on the third (dsi2) block, so > it > + should only have the "lp" clock > +minItems: 1 > +maxItems: 2 > + > + clock-names: > +oneOf: > + - items: > + - const: hs > + - const: lp > + - items: > + - const: lp > > All device trees have these in the right place, we just didn't notice that > the bindings were wrong exactly because we weren't using > formal YAML syntax. Now the strictness of this parser makes me > fix my bugs... > > > > + port: > > > +type: object > > > +description: > > > + A DPI port node with endpoint definitions as defined in > > > + Documentation/devicetree/bindings/media/video-interfaces.txt > > > + > > > + "#address-cells": > > > +const: 1 > > > + > > > + "#size-cells": > > > +const: 1 > > > + > > > + ranges: true > > > > This is a transition from .txt to DT Schema so OK with this sub-node. > > But otherwise the dsi node should have been linked using graph nodes. > > So OK - just thinking out loud. > > Actually when I introduced the MCDE DSI last year at first I used port > and graphs: > https://lore.kernel.org/dri-devel/20190207083647.20615-3-linus.wall...@linaro.org/ > Then Rob asked "why?": > https://lore.kernel.org/dri-devel/20190225223124.GA29057@bogus/ > And then I removed it, as having a panel directly under a > DSI host is fine. OK, thanks for the explanation. > > > > +patternProperties: > > > + "^dsi@[0-9a-f]+$": > > > +description: subnodes for the three DSI host adapters > > > +type: object > > > +allOf: > > > + - $ref: dsi-controller.yaml# > (...) > > The dsi nodes needs the #address-cells and #size-cells - at least if a > > panel node is specified. > > This is specified in the referenced schema dsi-controller.yaml. > > > > +required: > > > + - compatible > > > + - reg > > > + - interrupts > > > + - clocks > > > + - clock-names > > > + - epod-supply > > > + - vana-supply > > > + > > > +additionalProperties: true > > > > Why are additional properties allowed here? > > It's because the SoC peripherals have things like pin control > (currently handled by a quirk in the YAML validator I think) and > resets is something else I will likely add at some point, and then > this would result in warnings unless I lock-step changes in the > schema and DTS files. > > I *can* disallow this if you insist. Add reset now as an optional property and you are covered. The HW does not change, and this describes the HW and not the drivers. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH v2 4/5] drm/msm: add DRM_MSM_GEM_SYNC_CACHE for non-coherent cache maintenance
On Sat, Nov 14, 2020 at 12:10 PM Jonathan Marek wrote:
>
> On 11/14/20 2:39 PM, Rob Clark wrote:
> > On Sat, Nov 14, 2020 at 10:58 AM Jonathan Marek wrote:
> >>
> >> On 11/14/20 1:46 PM, Rob Clark wrote:
> >>> On Sat, Nov 14, 2020 at 8:24 AM Christoph Hellwig wrote:
>
> On Sat, Nov 14, 2020 at 10:17:12AM -0500, Jonathan Marek wrote:
> > +void msm_gem_sync_cache(struct drm_gem_object *obj, uint32_t flags,
> > + size_t range_start, size_t range_end)
> > +{
> > + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > + struct device *dev = msm_obj->base.dev->dev;
> > +
> > + /* exit early if get_pages() hasn't been called yet */
> > + if (!msm_obj->pages)
> > + return;
> > +
> > + /* TODO: sync only the specified range */
> > +
> > + if (flags & MSM_GEM_SYNC_FOR_DEVICE) {
> > + dma_sync_sg_for_device(dev, msm_obj->sgt->sgl,
> > + msm_obj->sgt->nents, DMA_TO_DEVICE);
> > + }
> > +
> > + if (flags & MSM_GEM_SYNC_FOR_CPU) {
> > + dma_sync_sg_for_cpu(dev, msm_obj->sgt->sgl,
> > + msm_obj->sgt->nents, DMA_FROM_DEVICE);
> > + }
>
> Splitting this helper from the only caller is rather strange, epecially
> with the two unused arguments. And I think the way this is specified
> to take a range, but ignoring it is actively dangerous. User space will
> rely on it syncing everything sooner or later and then you are stuck.
> So just define a sync all primitive for now, and if you really need a
> range sync and have actually implemented it add a new ioctl for that.
> >>>
> >>> We do already have a split of ioctl "layer" which enforces valid ioctl
> >>> params, etc, and gem (or other) module code which is called by the
> >>> ioctl func. So I think it is fine to keep this split here. (Also, I
> >>> think at some point there will be a uring type of ioctl alternative
> >>> which would re-use the same gem func.)
> >>>
> >>> But I do agree that the range should be respected or added later..
> >>> drm_ioctl() dispatch is well prepared for extending ioctls.
> >>>
> >>> And I assume there should be some validation that the range is aligned
> >>> to cache-line? Or can we flush a partial cache line?
> >>>
> >>
> >> The range is intended to be "sync at least this range", so that
> >> userspace doesn't have to worry about details like that.
> >>
> >
> > I don't think userspace can *not* worry about details like that.
> > Consider a case where the cpu and gpu are simultaneously accessing
> > different parts of a buffer (for ex, sub-allocation). There needs to
> > be cache-line separation between the two.
> >
>
> Right.. and it also seems like we can't get away with just
> flushing/invalidating the whole thing.
>
> qcom's vulkan driver has nonCoherentAtomSize=1, and it looks like
> dma_sync_single_for_cpu() does deal in some way with the partial cache
> line case, although I'm not sure that means we can have a
> nonCoherentAtomSize=1.
>
flush/inv the whole thing could be a useful first step, or at least I
can think of some uses for it. But if it isn't useful for how vk sees
the world, then maybe we should just implement the range properly from
the get-go. (And I *think* requiring the range to be aligned to
cacheline boundaries.. it is always easy from a kernel uabi PoV to
loosen restrictions later, than the other way around.)
BR,
-R
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/mediatek: dsi: Calculate horizontal_backporch_byte by itself
From: CK Hu
Using vm->hfront_porch + vm->hback_porch to calculate
horizontal_backporch_byte would make it negtive, so
use horizontal_backporch_byte itself to make it positive.
Fixes: 35bf948f1edb ("drm/mediatek: dsi: Fix scrolling of panel with small hfp
or hbp")
Signed-off-by: CK Hu
Signed-off-by: Chun-Kuang Hu
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 53 ++
1 file changed, 18 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 4a188a942c38..2a64fdaed9a7 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -444,7 +444,10 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
u32 horizontal_sync_active_byte;
u32 horizontal_backporch_byte;
u32 horizontal_frontporch_byte;
+ u32 horizontal_front_back_byte;
+ u32 data_phy_cycles_byte;
u32 dsi_tmp_buf_bpp, data_phy_cycles;
+ u32 delta;
struct mtk_phy_timing *timing = &dsi->phy_timing;
struct videomode *vm = &dsi->vm;
@@ -474,42 +477,22 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
data_phy_cycles = timing->lpx + timing->da_hs_prepare +
timing->da_hs_zero + timing->da_hs_exit;
- if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
- if ((vm->hfront_porch + vm->hback_porch) * dsi_tmp_buf_bpp >
- data_phy_cycles * dsi->lanes + 18) {
- horizontal_frontporch_byte =
- vm->hfront_porch * dsi_tmp_buf_bpp -
- (data_phy_cycles * dsi->lanes + 18) *
- vm->hfront_porch /
- (vm->hfront_porch + vm->hback_porch);
-
- horizontal_backporch_byte =
- horizontal_backporch_byte -
- (data_phy_cycles * dsi->lanes + 18) *
- vm->hback_porch /
- (vm->hfront_porch + vm->hback_porch);
- } else {
- DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
- horizontal_frontporch_byte = vm->hfront_porch *
-dsi_tmp_buf_bpp;
- }
+ delta = dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST ? 18 : 12;
+
+ horizontal_frontporch_byte = vm->hfront_porch * dsi_tmp_buf_bpp;
+ horizontal_front_back_byte = horizontal_frontporch_byte +
horizontal_backporch_byte;
+ data_phy_cycles_byte = data_phy_cycles * dsi->lanes + delta;
+
+ if (horizontal_front_back_byte > data_phy_cycles_byte) {
+ horizontal_frontporch_byte -= data_phy_cycles_byte *
+ horizontal_frontporch_byte /
+ horizontal_front_back_byte;
+
+ horizontal_backporch_byte -= data_phy_cycles_byte *
+horizontal_backporch_byte /
+horizontal_front_back_byte;
} else {
- if ((vm->hfront_porch + vm->hback_porch) * dsi_tmp_buf_bpp >
- data_phy_cycles * dsi->lanes + 12) {
- horizontal_frontporch_byte =
- vm->hfront_porch * dsi_tmp_buf_bpp -
- (data_phy_cycles * dsi->lanes + 12) *
- vm->hfront_porch /
- (vm->hfront_porch + vm->hback_porch);
- horizontal_backporch_byte = horizontal_backporch_byte -
- (data_phy_cycles * dsi->lanes + 12) *
- vm->hback_porch /
- (vm->hfront_porch + vm->hback_porch);
- } else {
- DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
- horizontal_frontporch_byte = vm->hfront_porch *
-dsi_tmp_buf_bpp;
- }
+ DRM_WARN("HFP + HBP less than d-phy, FPS will under 60Hz\n");
}
writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/mediatek: dsi: Calculate horizontal_backporch_byte by itself
Hi, Bilal:
Please help to test this patch on your Chromebook elm, thanks.
Regards,
Chun-Kuang Hu
Chun-Kuang Hu 於 2020年11月15日 週日 上午8:14寫道：
>
> From: CK Hu
>
> Using vm->hfront_porch + vm->hback_porch to calculate
> horizontal_backporch_byte would make it negtive, so
> use horizontal_backporch_byte itself to make it positive.
>
> Fixes: 35bf948f1edb ("drm/mediatek: dsi: Fix scrolling of panel with small
> hfp or hbp")
>
> Signed-off-by: CK Hu
> Signed-off-by: Chun-Kuang Hu
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 53 ++
> 1 file changed, 18 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 4a188a942c38..2a64fdaed9a7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -444,7 +444,10 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi
> *dsi)
> u32 horizontal_sync_active_byte;
> u32 horizontal_backporch_byte;
> u32 horizontal_frontporch_byte;
> + u32 horizontal_front_back_byte;
> + u32 data_phy_cycles_byte;
> u32 dsi_tmp_buf_bpp, data_phy_cycles;
> + u32 delta;
> struct mtk_phy_timing *timing = &dsi->phy_timing;
>
> struct videomode *vm = &dsi->vm;
> @@ -474,42 +477,22 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi
> *dsi)
> data_phy_cycles = timing->lpx + timing->da_hs_prepare +
> timing->da_hs_zero + timing->da_hs_exit;
>
> - if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> - if ((vm->hfront_porch + vm->hback_porch) * dsi_tmp_buf_bpp >
> - data_phy_cycles * dsi->lanes + 18) {
> - horizontal_frontporch_byte =
> - vm->hfront_porch * dsi_tmp_buf_bpp -
> - (data_phy_cycles * dsi->lanes + 18) *
> - vm->hfront_porch /
> - (vm->hfront_porch + vm->hback_porch);
> -
> - horizontal_backporch_byte =
> - horizontal_backporch_byte -
> - (data_phy_cycles * dsi->lanes + 18) *
> - vm->hback_porch /
> - (vm->hfront_porch + vm->hback_porch);
> - } else {
> - DRM_WARN("HFP less than d-phy, FPS will under
> 60Hz\n");
> - horizontal_frontporch_byte = vm->hfront_porch *
> -dsi_tmp_buf_bpp;
> - }
> + delta = dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST ? 18 : 12;
> +
> + horizontal_frontporch_byte = vm->hfront_porch * dsi_tmp_buf_bpp;
> + horizontal_front_back_byte = horizontal_frontporch_byte +
> horizontal_backporch_byte;
> + data_phy_cycles_byte = data_phy_cycles * dsi->lanes + delta;
> +
> + if (horizontal_front_back_byte > data_phy_cycles_byte) {
> + horizontal_frontporch_byte -= data_phy_cycles_byte *
> + horizontal_frontporch_byte /
> + horizontal_front_back_byte;
> +
> + horizontal_backporch_byte -= data_phy_cycles_byte *
> +horizontal_backporch_byte /
> +horizontal_front_back_byte;
> } else {
> - if ((vm->hfront_porch + vm->hback_porch) * dsi_tmp_buf_bpp >
> - data_phy_cycles * dsi->lanes + 12) {
> - horizontal_frontporch_byte =
> - vm->hfront_porch * dsi_tmp_buf_bpp -
> - (data_phy_cycles * dsi->lanes + 12) *
> - vm->hfront_porch /
> - (vm->hfront_porch + vm->hback_porch);
> - horizontal_backporch_byte = horizontal_backporch_byte
> -
> - (data_phy_cycles * dsi->lanes + 12) *
> - vm->hback_porch /
> - (vm->hfront_porch + vm->hback_porch);
> - } else {
> - DRM_WARN("HFP less than d-phy, FPS will under
> 60Hz\n");
> - horizontal_frontporch_byte = vm->hfront_porch *
> -dsi_tmp_buf_bpp;
> - }
> + DRM_WARN("HFP + HBP less than d-phy, FPS will under 60Hz\n");
> }
>
> writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC);
> --
> 2.17.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/8] drm: Add dummy page per device or GEM object
On 11/14/20 4:51 AM, Daniel Vetter wrote:
On Sat, Nov 14, 2020 at 9:41 AM Christian König
wrote:
Am 13.11.20 um 21:52 schrieb Andrey Grodzovsky:
On 6/22/20 1:50 PM, Daniel Vetter wrote:
On Mon, Jun 22, 2020 at 7:45 PM Christian König
wrote:
Am 22.06.20 um 16:32 schrieb Andrey Grodzovsky:
On 6/22/20 9:18 AM, Christian König wrote:
Am 21.06.20 um 08:03 schrieb Andrey Grodzovsky:
Will be used to reroute CPU mapped BO's page faults once
device is removed.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/drm_file.c | 8
drivers/gpu/drm/drm_prime.c | 10 ++
include/drm/drm_file.h | 2 ++
include/drm/drm_gem.h | 2 ++
4 files changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index c4c704e..67c0770 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -188,6 +188,12 @@ struct drm_file *drm_file_alloc(struct
drm_minor *minor)
goto out_prime_destroy;
}
+file->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+if (!file->dummy_page) {
+ret = -ENOMEM;
+goto out_prime_destroy;
+}
+
return file;
out_prime_destroy:
@@ -284,6 +290,8 @@ void drm_file_free(struct drm_file *file)
if (dev->driver->postclose)
dev->driver->postclose(dev, file);
+__free_page(file->dummy_page);
+
drm_prime_destroy_file_private(&file->prime);
WARN_ON(!list_empty(&file->event_list));
diff --git a/drivers/gpu/drm/drm_prime.c
b/drivers/gpu/drm/drm_prime.c
index 1de2cde..c482e9c 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -335,6 +335,13 @@ int drm_gem_prime_fd_to_handle(struct
drm_device *dev,
ret = drm_prime_add_buf_handle(&file_priv->prime,
dma_buf, *handle);
+
+if (!ret) {
+obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+if (!obj->dummy_page)
+ret = -ENOMEM;
+}
+
While the per file case still looks acceptable this is a clear NAK
since it will massively increase the memory needed for a prime
exported object.
I think that this is quite overkill in the first place and for the
hot unplug case we can just use the global dummy page as well.
Christian.
Global dummy page is good for read access, what do you do on write
access ? My first approach was indeed to map at first global dummy
page as read only and mark the vma->vm_flags as !VM_SHARED assuming
that this would trigger Copy On Write flow in core mm
(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.7-rc7%2Fsource%2Fmm%2Fmemory.c%23L3977&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C00053e9d983041ed63ae08d2ed87%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637409443224016377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=kghiG3VpCJod6YefExoDVPl9X03zNhw3SN5GAxgbnmU%3D&reserved=0)
on the next page fault to same address triggered by a write access but
then i realized a new COW page will be allocated for each such mapping
and this is much more wasteful then having a dedicated page per GEM
object.
Yeah, but this is only for a very very small corner cases. What we need
to prevent is increasing the memory usage during normal operation to
much.
Using memory during the unplug is completely unproblematic because we
just released quite a bunch of it by releasing all those system memory
buffers.
And I'm pretty sure that COWed pages are correctly accounted towards
the
used memory of a process.
So I think if that approach works as intended and the COW pages are
released again on unmapping it would be the perfect solution to the
problem.
Daniel what do you think?
If COW works, sure sounds reasonable. And if we can make sure we
managed to drop all the system allocations (otherwise suddenly 2x
memory usage, worst case). But I have no idea whether we can
retroshoehorn that into an established vma, you might have fun stuff
like a mkwrite handler there (which I thought is the COW handler
thing, but really no idea).
If we need to massively change stuff then I think rw dummy page,
allocated on first fault after hotunplug (maybe just make it one per
object, that's simplest) seems like the much safer option. Much less
code that can go wrong.
-Daniel
Regarding COW, i was looking into how to properly implement it from
within the fault handler (i.e. ttm_bo_vm_fault)
and the main obstacle I hit is that of exclusive access to the
vm_area_struct, i need to be able to modify
vma->vm_flags (and vm_page_prot) to remove VM_SHARED bit so COW can
be triggered on subsequent write access
fault (here
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fmm%2Fmemory.c%23L4128&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C00053e9d983041ed63ae08d2ed87%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637409443224016377%7CUnknow
Re: [PATCH v2 1/8] drm: Add dummy page per device or GEM object
On 6/22/20 5:35 AM, Daniel Vetter wrote:
On Sun, Jun 21, 2020 at 02:03:01AM -0400, Andrey Grodzovsky wrote:
Will be used to reroute CPU mapped BO's page faults once
device is removed.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/drm_file.c | 8
drivers/gpu/drm/drm_prime.c | 10 ++
include/drm/drm_file.h | 2 ++
include/drm/drm_gem.h | 2 ++
4 files changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index c4c704e..67c0770 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -188,6 +188,12 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
goto out_prime_destroy;
}
+ file->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+ if (!file->dummy_page) {
+ ret = -ENOMEM;
+ goto out_prime_destroy;
+ }
+
return file;
out_prime_destroy:
@@ -284,6 +290,8 @@ void drm_file_free(struct drm_file *file)
if (dev->driver->postclose)
dev->driver->postclose(dev, file);
+ __free_page(file->dummy_page);
+
drm_prime_destroy_file_private(&file->prime);
WARN_ON(!list_empty(&file->event_list));
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 1de2cde..c482e9c 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -335,6 +335,13 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
ret = drm_prime_add_buf_handle(&file_priv->prime,
dma_buf, *handle);
+
+ if (!ret) {
+ obj->dummy_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+ if (!obj->dummy_page)
+ ret = -ENOMEM;
+ }
+
mutex_unlock(&file_priv->prime.lock);
if (ret)
goto fail;
@@ -1006,6 +1013,9 @@ void drm_prime_gem_destroy(struct drm_gem_object *obj,
struct sg_table *sg)
dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
dma_buf = attach->dmabuf;
dma_buf_detach(attach->dmabuf, attach);
+
+ __free_page(obj->dummy_page);
+
/* remove the reference */
dma_buf_put(dma_buf);
}
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 19df802..349a658 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -335,6 +335,8 @@ struct drm_file {
*/
struct drm_prime_file_private prime;
Kerneldoc for these please, including why we need them and when. E.g. the
one in gem_bo should say it's only for exported buffers, so that we're not
colliding security spaces.
+ struct page *dummy_page;
+
/* private: */
#if IS_ENABLED(CONFIG_DRM_LEGACY)
unsigned long lock_count; /* DRI1 legacy lock count */
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 0b37506..47460d1 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -310,6 +310,8 @@ struct drm_gem_object {
*
*/
const struct drm_gem_object_funcs *funcs;
+
+ struct page *dummy_page;
};
I think amdgpu doesn't care, but everyone else still might care somewhat
about flink. That also shares buffers, so also needs to allocate the
per-bo dummy page.
Not familiar with FLINK so I read a bit here https://lwn.net/Articles/283798/
sections 3 and 4 about FLINK naming and later mapping, I don't see a difference
between FLINK and local BO mapping as opening by FLINK name returns handle
to the same BO as the original. Why then we need a special handling for FLINK ?
Andrey
I also wonder whether we shouldn't have a helper to look up the dummy
page, just to encode in core code how it's supposedo to cascade.
-Daniel
/**
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
