Re: [PATCH 1/6] gpu: host1x: Fixes to host1x firewall
On Fri, May 17, 2013 at 02:49:43PM +0300, Arto Merilainen wrote:
> From: Terje Bergstrom
>
> This patch adds several fixes to host1x firewall:
> - Host1x firewall does not survive if it expects a reloc, but user
> space didn't pass any relocs. Also it reset the reloc table for
> each gather, whereas they should be reset only per submit. Also
> class does not need to be reset for each class - once per submit
> is enough.
> - For INCR opcode the check was not working properly at all.
> - The firewall verified gather buffers before copying them. This
> allowed a malicious application to rewrite the buffer content by
> timing the rewrite carefully. This patch makes the buffer
> validation occur after copying the buffers.
Can these be split into separate patches, please? It's not only always
good to split logical changes into separate patches but it also makes
reviewing a lot more pleasant. It's hard to tell from this combined
patch which changes belong together.
I have a few additional comments inline.
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index f665d67..4f3c004 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job,
> struct host1x_bo *cmdbuf)
> void *cmdbuf_page_addr = NULL;
>
> /* pin & patch the relocs for one gather */
> - while (i < job->num_relocs) {
> + for (i = 0; i < job->num_relocs; ++i) {
Nit: I prefer post-increment where possible. For consistency.
> @@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job,
> struct host1x_bo *cmdbuf)
> return 0;
> }
>
> -static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
> -unsigned int offset)
> +static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
> + unsigned int offset)
> {
> offset *= sizeof(u32);
>
> - if (reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
> - return -EINVAL;
> + if (!reloc || reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
Is the additional !reloc check really necessary? Looking at the callers,
they always pass in fw->relocarray, which in turn is only NULL if no
buffers are to be relocated.
> + return true;
>
> - return 0;
> + return false;
> }
I wonder whether we should be changing the logic here and have the
check_reloc() function return true if the relocation is good. I find
that to be more intuitive.
> @@ -376,69 +371,58 @@ static int check_nonincr(struct host1x_firewall *fw)
> return 0;
> }
>
> -static int validate(struct host1x_job *job, struct device *dev,
> - struct host1x_job_gather *g)
> +static int validate_gather(struct host1x_firewall *fw,
> +struct host1x_job_gather *g)
I don't think we necessarily need to rename the function. However since
you modify each line that the rename touches anyway it's okay.
> @@ -508,6 +502,7 @@ int host1x_job_pin(struct host1x_job *job, struct device
> *dev)
> int err;
> unsigned int i, j;
> struct host1x *host = dev_get_drvdata(dev->parent);
> +
> DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
This is an unnecessary whitespace change.
Thierry
pgpMd8pehVx5s.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/6] gpu: host1x: Fix syncpoint wait return value
On Fri, May 17, 2013 at 02:49:44PM +0300, Arto Merilainen wrote: > Syncpoint wait returned EAGAIN if it was called with zero timeout. > This patch modifies the function to return ETIMEDOUT. This description is a bit redundant, because it repeats in prose what the code does. I'd rather see a description of why the change is necessary. Thinking about it, maybe it would be good to have two separate error codes. Keeping -EAGAIN for the case where a zero timeout was passed doesn't sound too bad to differentiate it from the case where a non- zero timeout was passed and it actually timed out. What do you think? Thierry pgpVRG7ijonV2.pgp Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/6] gpu: host1x: Fix memory access in syncpt request
On Fri, May 17, 2013 at 02:49:45PM +0300, Arto Merilainen wrote: > This patch fixes a bad memory access in syncpoint request code. If > no syncpoints were available, the code accessed unreserved memory > area causing unexpected behaviour. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/syncpt.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c > index 5bf5366..6b7ee88 100644 > --- a/drivers/gpu/host1x/syncpt.c > +++ b/drivers/gpu/host1x/syncpt.c > @@ -40,7 +40,7 @@ static struct host1x_syncpt *_host1x_syncpt_alloc(struct > host1x *host, > > for (i = 0; i < host->info->nb_pts && sp->name; i++, sp++) > ; > - if (sp->dev) > + if (i >= host->info->nb_pts) > return NULL; While changing this, can you please add a blank line between the loop and the new 'if (...)'? Thierry pgpN5Hl94dJ5j.pgp Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/6] gpu: host1x: Fix client_managed type
On Fri, May 17, 2013 at 02:49:46PM +0300, Arto Merilainen wrote: > client_managed field in syncpoint structure was defined as an > integer. The field holds, however, only a boolean value. This patch > modifies the type to boolean. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/drm/gr2d.c |2 +- > drivers/gpu/host1x/syncpt.c |8 > drivers/gpu/host1x/syncpt.h |6 +++--- > 3 files changed, 8 insertions(+), 8 deletions(-) Looks good. I'll wait for v2 of the series before applying this to make things easier. Thierry pgpb7vTijEwNe.pgp Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/6] gpu: host1x: Rework CPU syncpoint increment
On Fri, May 17, 2013 at 02:49:47PM +0300, Arto Merilainen wrote: > This patch merges host1x_syncpt_cpu_incr to host1x_syncpt_incr() as > they are in practise doing the same thing. host1x_syncpt_incr() is also > modified to return error codes. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/dev.h |8 > drivers/gpu/host1x/hw/cdma_hw.c |2 +- > drivers/gpu/host1x/hw/syncpt_hw.c | 12 +--- > drivers/gpu/host1x/syncpt.c | 15 ++- > drivers/gpu/host1x/syncpt.h |7 ++- > 5 files changed, 14 insertions(+), 30 deletions(-) Looks good. Can you carry this over to v2 as well so I can apply all of them in one go? Thierry pgpa9AftkFihZ.pgp Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/6] drm/tegra: Fix syncpoint increment return code
On Fri, May 17, 2013 at 02:49:48PM +0300, Arto Merilainen wrote: > Add syncpoint increment to return a proper return code based on the > return value from host1x. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/drm/drm.c |3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Maybe this should be folded into patch 5 because it is related to the fact that host1x_syncpt_incr() now returns proper error codes? Thierry pgpxSZgry01y4.pgp Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Commit "drm: run the hpd irq event code directly" causes stutter from repeated EDID retrievals
On Sun, May 26, 2013 at 5:38 PM, Gard Spreemann wrote: > Hi, > > If I should contact a list instead of you directly, I sincerely apologize! You should have cc'ed relevant lists at least ;-) Fixed up. > After a recent kernel upgrade, I found that my system (using Nouveau) would > stutter whenever eDP-1 was turned off. That display being turned off > apparently > causes repeated EDID retrievals and failed checksums. I did a git bisect which > lead me to > *** > commit 69787f7da6b2adc4054357a661aaa1701a9ca76f > Author: Daniel Vetter > Date: Tue Oct 23 18:23:34 2012 + > > drm: run the hpd irq event code directly > > All drivers already have a work item to run the hpd code, so we don't > need to launch a new one in the helper code. Dave Airlie mentioned > that the cancel+re-queue might paper over DP related hpd ping-pongs, > hence why this is split out. > > Signed-off-by: Daniel Vetter > Reviewed-by: Alex Deucher > Signed-off-by: Dave Airlie > *** > as the first bad commit, and I was advised to contact you as well as file a > bug > with the Nouveau project [1]. > > I don't have enough insight into the code in question to understand the > problem, but your discussion on April 6 [2] seems relevant. > > Please let me know if there's anything I can do to help. > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=64858 > [2] https://patchwork.kernel.org/patch/2402211/ My guess is that you've worked around these edid stalls with drm_kms_helper.poll=0 and the above commit breaks that w/a. That patch was shot down unfortunately. If you can't reproduce these stalls on older kernels when removing the poll=0 option, then something else broke. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Commit "drm: run the hpd irq event code directly" causes stutter from repeated EDID retrievals
On Sun, May 26, 2013 at 6:25 PM, Gard Spreemann wrote: > On Sunday 26 May 2013 18:14:06 Daniel Vetter wrote: >> My guess is that you've worked around these edid stalls with >> drm_kms_helper.poll=0 and the above commit breaks that w/a. That patch >> was shot down unfortunately. If you can't reproduce these stalls on >> older kernels when removing the poll=0 option, then something else >> broke. > > You are correct. Before that commit, the stuttering behavior is present with > poll=1, but not with poll=0. Starting with that commit, the behavior is > present nomatter the poll setting. > > Am I correct in thinking that this is a problem without a clear solution? It's a problem with a solution rejected by Dave and Alex, and I tend to agree with them a bit. The real solution is to grow nouveau/radeon hpd storm mitigation code (i915 already has it in 3.10), since it's much better to cut of the issue at the source (by disasbling hpd interrupts completely for such problematic outputs) instead of trying to work around the worst side-effects of getting too many. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm-openchrome status (or will it be in Kernel 3.10?)
> wrote: > > Am Dienstag, 30. April 2013, 06:06:22 schrieb Dave Airlie: > >> On Tue, Apr 30, 2013 at 2:17 AM, Johannes Obermayr > >> wrote: > >> > Hi James, > >> > > >> > Linus recently released Kernel 3.9, merge window for Kernel 3.10 has > >> > been opened, and the question is whether drm-openchrome will be part of > >> > the new Kernel version. > >> > >> Johannes, > >> > >> you misunderstand merge window. The merge window is for stuff to go > >> from toplevel maintainers to Linus, not for new stuff to appear and be > >> merged. > > > > Dave, > > > > I know you maintain also a merge window for drm stuff which starts at ~ rc6. > > > > But I am unsure when it closes: When Linus opens his merge window or when > > you forward your main drm pull request to Linus. > > First case means it is definitely too late for drm-openchrome in 3.10. > > Second case means there can be hope (depending on James' answer) ... > > > > But regarding your answer I assume first case is right. > > Once Linus opens his window, I won't accept anything major that hasn't > been posted to the list for review before. > > I generally have a lag time of hoovering up on the list stuff, but > something like this that has never been posted would have no hope. Hello. Sorry I didn't responded earlier. I was swamped at work with projects and conferences. After I got back from my last conference Xavier informed me about this email and we had a discussion about merging. Currently we have a external tree which is two years old but most of the work has happened in the last 8 months. The reason for delaying mergering was to ensure most distros would be carrying the version of openchrome xorg driver that supports KMS. Today with that barrier gone we have to deal with the drm-openchrome branch not being in top shape. Since I have been back the tree has been cleaned up and the last show stoppers were checked in yesterday. Currently we support LVDS, HDMI/DVI, and VGA outputs which cover most people. By no means is the kernel driver complete but it is usable. I would say the branch is now ready for merger. Dave what is the best way to push this code? I am in no rush so I like to do it the right way. We have scattered history in the external tree but I would not lose sleep over its lost. I could just post pieces of the code to this list for review. One choice is after all the code is reviewed is to then merge the branch. Another choice is to create a new branch for later merge that will be composed of the patches posted to this list. We lose the history but that is not to be of deal for me. Just let me know what you would like and I will start the process. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/gma500: Fix cursor gem obj referencing on psb
The internal crtc cursor gem object pointer was never set/updated since
it was required to be set in the first place.
Fixing this will make the pin/unpin count match and prevent cursor
objects from leaking when userspace drops all references to it. Also
make sure we drop the gem obj reference on failure.
This patch only affects Poulsbo chips.
Signed-off-by: Patrik Jakobsson
---
drivers/gpu/drm/gma500/psb_intel_display.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c
b/drivers/gpu/drm/gma500/psb_intel_display.c
index 6e8f42b..c4a39da 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -843,7 +843,7 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc *crtc,
struct gtt_range *cursor_gt = psb_intel_crtc->cursor_gt;
struct drm_gem_object *obj;
void *tmp_dst, *tmp_src;
- int ret, i, cursor_pages;
+ int ret = 0, i, cursor_pages;
/* if we want to turn of the cursor ignore width and height */
if (!handle) {
@@ -880,7 +880,8 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc *crtc,
if (obj->size < width * height * 4) {
dev_dbg(dev->dev, "buffer is to small\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto unref_cursor;
}
gt = container_of(obj, struct gtt_range, gem);
@@ -889,13 +890,14 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc
*crtc,
ret = psb_gtt_pin(gt);
if (ret) {
dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
- return ret;
+ goto unref_cursor;
}
if (dev_priv->ops->cursor_needs_phys) {
if (cursor_gt == NULL) {
dev_err(dev->dev, "No hardware cursor mem available");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto unref_cursor;
}
/* Prevent overflow */
@@ -936,9 +938,14 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc *crtc,
struct gtt_range, gem);
psb_gtt_unpin(gt);
drm_gem_object_unreference(psb_intel_crtc->cursor_obj);
- psb_intel_crtc->cursor_obj = obj;
}
- return 0;
+
+ psb_intel_crtc->cursor_obj = obj;
+ return ret;
+
+unref_cursor:
+ drm_gem_object_unreference(obj);
+ return ret;
}
static int psb_intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
--
1.8.1.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/gma500: Fix cursor gem obj referencing on cdv
The internal crtc cursor gem object pointer was never set/updated since
it was required to be set in the first place.
Fixing this will make the pin/unpin count match and prevent cursor
objects from leaking when userspace drops all references to it. Also
make sure we drop the gem obj reference on failure.
This patch only affects Cedarview chips.
Signed-off-by: Patrik Jakobsson
---
drivers/gpu/drm/gma500/cdv_intel_display.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c
b/drivers/gpu/drm/gma500/cdv_intel_display.c
index 3cfd093..f30d00f 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_display.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_display.c
@@ -1462,7 +1462,7 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
size_t addr = 0;
struct gtt_range *gt;
struct drm_gem_object *obj;
- int ret;
+ int ret = 0;
/* if we want to turn of the cursor ignore width and height */
if (!handle) {
@@ -1499,7 +1499,8 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
if (obj->size < width * height * 4) {
dev_dbg(dev->dev, "buffer is to small\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto unref_cursor;
}
gt = container_of(obj, struct gtt_range, gem);
@@ -1508,7 +1509,7 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
ret = psb_gtt_pin(gt);
if (ret) {
dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
- return ret;
+ goto unref_cursor;
}
addr = gt->offset; /* Or resource.start ??? */
@@ -1532,9 +1533,14 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
struct gtt_range, gem);
psb_gtt_unpin(gt);
drm_gem_object_unreference(psb_intel_crtc->cursor_obj);
- psb_intel_crtc->cursor_obj = obj;
}
- return 0;
+
+ psb_intel_crtc->cursor_obj = obj;
+ return ret;
+
+unref_cursor:
+ drm_gem_object_unreference(obj);
+ return ret;
}
static int cdv_intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
--
1.8.1.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
A framebuffer is pinned when it's set as pipe base, so we also need to unpin it when it's destroyed. Some framebuffers are never used as scanout (no mode set on them) so if the pin count is less than one, no unpinning is needed. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 Signed-off-by: Patrik Jakobsson --- drivers/gpu/drm/gma500/framebuffer.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 8b1b6d9..1a11b69 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct drm_framebuffer *fb) /* Let DRM do its clean up */ drm_framebuffer_cleanup(fb); + + /* Unpin any psb_intel_pipe_set_base() pinning */ + if (r->in_gart > 0) + psb_gtt_unpin(r); + /* We are no longer using the resource in GEM */ drm_gem_object_unreference_unlocked(&r->gem); kfree(fb); -- 1.8.1.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
On Sun, May 26, 2013 at 08:03:53PM +0200, Patrik Jakobsson wrote: > A framebuffer is pinned when it's set as pipe base, so we also need to > unpin it when it's destroyed. Some framebuffers are never used as > scanout (no mode set on them) so if the pin count is less than one, no > unpinning is needed. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 > Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 > > Signed-off-by: Patrik Jakobsson > --- > drivers/gpu/drm/gma500/framebuffer.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/gma500/framebuffer.c > b/drivers/gpu/drm/gma500/framebuffer.c > index 8b1b6d9..1a11b69 100644 > --- a/drivers/gpu/drm/gma500/framebuffer.c > +++ b/drivers/gpu/drm/gma500/framebuffer.c > @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct > drm_framebuffer *fb) > > /* Let DRM do its clean up */ > drm_framebuffer_cleanup(fb); > + > + /* Unpin any psb_intel_pipe_set_base() pinning */ > + if (r->in_gart > 0) > + psb_gtt_unpin(r); The drm core /should/ have removed the user framebuffer from all active users before it calls down into your ->destroy callback. Why can't gma500 just pin/unpin on demand when the buffer is actually in use? If a pin survives the official use as seen by the drm core (e.g. to keep a buffer around until the pageflip completes) you can simply keep an additional reference around. Cheers, Daniel > + > /* We are no longer using the resource in GEM */ > drm_gem_object_unreference_unlocked(&r->gem); > kfree(fb); > -- > 1.8.1.2 > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter wrote: > On Sun, May 26, 2013 at 08:03:53PM +0200, Patrik Jakobsson wrote: >> A framebuffer is pinned when it's set as pipe base, so we also need to >> unpin it when it's destroyed. Some framebuffers are never used as >> scanout (no mode set on them) so if the pin count is less than one, no >> unpinning is needed. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 >> Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 >> >> Signed-off-by: Patrik Jakobsson >> --- >> drivers/gpu/drm/gma500/framebuffer.c | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/gma500/framebuffer.c >> b/drivers/gpu/drm/gma500/framebuffer.c >> index 8b1b6d9..1a11b69 100644 >> --- a/drivers/gpu/drm/gma500/framebuffer.c >> +++ b/drivers/gpu/drm/gma500/framebuffer.c >> @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct >> drm_framebuffer *fb) >> >> /* Let DRM do its clean up */ >> drm_framebuffer_cleanup(fb); >> + >> + /* Unpin any psb_intel_pipe_set_base() pinning */ >> + if (r->in_gart > 0) >> + psb_gtt_unpin(r); > > The drm core /should/ have removed the user framebuffer from all active > users before it calls down into your ->destroy callback. Why can't gma500 > just pin/unpin on demand when the buffer is actually in use? Thanks for the feedback DRM core does correctly keep track of the users but the last ref is dropped in our ->destroy callback and at that point it's still pinned from pipe_set_base(). So if it's considered too late to unpin the framebuffer in destroy, we never had it right in the first place. Not a big surprise I guess :) > If a pin survives the official use as seen by the drm core (e.g. to keep a > buffer around until the pageflip completes) you can simply keep an > additional reference around. As I wrote above, that additional reference is not dropped until ->destroy anyways and we don't have page flipping support and to be honest I haven't even looked at implementing it yet. So the logical point to release the pin would be in pipe_set_base() (or am I wrong?) by doing an unpin on the old_fb. Problem is that when killing X and switching back to fbdev we get no old_fb. I might be missing something, so any suggestions are welcome. Thanks Patrik ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 65016] New: [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 Priority: medium Bug ID: 65016 Assignee: dri-devel@lists.freedesktop.org Summary: [radeonsi] X server segfaults at startup with mesa-9.1* Severity: normal Classification: Unclassified OS: All Reporter: alexan...@tsoy.me Hardware: Other Status: NEW Version: 9.1 Component: Drivers/Gallium/radeonsi Product: Mesa X server segfaults at startup whith mesa 9.1*. No problems with Git snapshots of mesa (these are the only versions of mesa which works on my system). I'll attach two backtraces below: 1. llvm and mesa was compiled with debugging support. Unfortunately this trace is incomplete =/ 2. llvm was compiled without debugging support, mesa was compiled whith debugging support. This trace is complete, but lacks a lot of debugging info. Hardware: 01:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI Cape Verde PRO [Radeon HD 7750] Subsystem: Giga-byte Technology Device 2260 Kernel driver in use: radeon Kernel modules: radeon Software: - mesa 9.1.3, also tried earlier minor versions - llvm 3.3_rc2, llvm Git - xorg-server-1.13.4 - xf86-video-ati-7.1.0 - glamor Git - linux 3.8.12 -- You are receiving this mail because: You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 65016] [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 --- Comment #1 from Alexander Tsoy --- Created attachment 79818 --> https://bugs.freedesktop.org/attachment.cgi?id=79818&action=edit [1] - both llvm and mesa with debugging support -- You are receiving this mail because: You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 65016] [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 --- Comment #2 from Alexander Tsoy --- Created attachment 79819 --> https://bugs.freedesktop.org/attachment.cgi?id=79819&action=edit backtrace [2] - only mesa with debugging support -- You are receiving this mail because: You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 65016] [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 Alexander Tsoy changed: What|Removed |Added Attachment #79818|[1] - both llvm and mesa|backtrace [1] - both llvm description|with debugging support |and mesa with debugging ||support -- You are receiving this mail because: You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
On Sun, May 26, 2013 at 10:31 PM, Patrik Jakobsson wrote: > On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter wrote: >> On Sun, May 26, 2013 at 08:03:53PM +0200, Patrik Jakobsson wrote: >>> A framebuffer is pinned when it's set as pipe base, so we also need to >>> unpin it when it's destroyed. Some framebuffers are never used as >>> scanout (no mode set on them) so if the pin count is less than one, no >>> unpinning is needed. >>> >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 >>> Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 >>> >>> Signed-off-by: Patrik Jakobsson >>> --- >>> drivers/gpu/drm/gma500/framebuffer.c | 5 + >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/gma500/framebuffer.c >>> b/drivers/gpu/drm/gma500/framebuffer.c >>> index 8b1b6d9..1a11b69 100644 >>> --- a/drivers/gpu/drm/gma500/framebuffer.c >>> +++ b/drivers/gpu/drm/gma500/framebuffer.c >>> @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct >>> drm_framebuffer *fb) >>> >>> /* Let DRM do its clean up */ >>> drm_framebuffer_cleanup(fb); >>> + >>> + /* Unpin any psb_intel_pipe_set_base() pinning */ >>> + if (r->in_gart > 0) >>> + psb_gtt_unpin(r); >> >> The drm core /should/ have removed the user framebuffer from all active >> users before it calls down into your ->destroy callback. Why can't gma500 >> just pin/unpin on demand when the buffer is actually in use? > > Thanks for the feedback > > DRM core does correctly keep track of the users but the last ref is dropped in > our ->destroy callback and at that point it's still pinned from > pipe_set_base(). > So if it's considered too late to unpin the framebuffer in destroy, we never > had > it right in the first place. Not a big surprise I guess :) > >> If a pin survives the official use as seen by the drm core (e.g. to keep a >> buffer around until the pageflip completes) you can simply keep an >> additional reference around. > > As I wrote above, that additional reference is not dropped until ->destroy > anyways and we don't have page flipping support and to be honest I haven't > even > looked at implementing it yet. So the logical point to release the pin would > be > in pipe_set_base() (or am I wrong?) by doing an unpin on the old_fb. Problem > is > that when killing X and switching back to fbdev we get no old_fb. > > I might be missing something, so any suggestions are welcome. Dumping our irc discussion for google to index here: I sounds like gma500 code is missing the crtc disabling sequence when X shuts down, i.e. the crtc on->off transition. Then when you switch to fbcon you only get a crtc off->on state transition and so don't see an old fb in set_base, which means that the pin refcount of the old framebuffer is lost. To fix this you can use the special call to crtc_funcs->disable (instead of the default crtc_funcs->dpms(OFF)) in drm_helper_disable_unused_functions. Note that X could also do the vt switch first and then only do the framebuffer destruction, in which case I think your patch here would drop the pin reference twice: Once in set_base since we have an old_fb and once in the framebuffer destroy callback. See intel_crtc_disable in intel_display.c in a v3.6 kernel version for how drm/i915 cleanup up the fb pin reference before we've reworked our modeset code and switched away from the crtc helpers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[REGRESSION] system does not resume from ram due to commit "drm/nv50/fifo: prevent races between clients updating playlists"
My NV96 does not resume from suspend to ram (the screen stays black, magic sysrq keys do work) with the current linus git kernel, i bisected it to the following commit. drm/nv50/fifo: prevent races between clients updating playlists b5096566f6e1ee2b88324772f020ae9bc0cfa9a0 It's not obvious to me how this causes problems, but reverting this commit does solve my problem. -- Far away from the primal instinct, the song seems to fade away, the river get wider between your thoughts and the things we do and say. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm-openchrome status (or will it be in Kernel 3.10?)
> Sorry I didn't responded earlier. I was swamped at work with > projects and conferences. After I got back from my last conference > Xavier informed me about this email and we had a discussion about > merging. Currently we have a external tree which is two years old but > most of the work has happened in the last 8 months. The reason for > delaying mergering was to ensure most distros would be carrying the > version of openchrome xorg driver that supports KMS. Today with that > barrier gone we have to deal with the drm-openchrome branch not being > in top shape. Since I have been back the tree has been cleaned up and > the last show stoppers were checked in yesterday. Currently we support > LVDS, HDMI/DVI, and VGA outputs which cover most people. By no means is > the kernel driver complete but it is usable. I would say the branch is > now ready for merger. > Dave what is the best way to push this code? I am in no rush so > I like to do it the right way. We have scattered history in the external > tree but I would not lose sleep over its lost. I could just post pieces > of the code to this list for review. One choice is after all the code is > reviewed is to then merge the branch. Another choice is to create a new > branch for later merge that will be composed of the patches posted to this > list. We lose the history but that is not to be of deal for me. Just let > me know what you would like and I will start the process. Hi James, I would probably say history loss is inevitable unless the tree is at a fairly high standard wrt bisection fail etc. But it would probably be good to post the driver to dri-devel at least once so people can take a look, not sure what the best way to split it up is, the initial patches can be split different than what we merge if it makes reading it easier. Can you post a link to what you consider the best place for me to be looking so I can see if splitting it makes sense. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3, part2 14/20] PCI, DRM: use hotplug-safe iterators to walk PCI buses
Enhance DRM drvier to use hotplug-safe iterators to walk PCI buses.
Signed-off-by: Jiang Liu
Cc: David Airlie
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/drm_fops.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 429e07d..dc8c40b 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -359,9 +359,11 @@ static int drm_open_helper(struct inode *inode, struct
file *filp,
pci_dev_put(pci_dev);
}
if (!dev->hose) {
- struct pci_bus *b = pci_bus_b(pci_root_buses.next);
- if (b)
+ struct pci_bus *b = pci_get_next_root_bus(NULL);
+ if (b) {
dev->hose = b->sysdata;
+ pci_bus_put(b);
+ }
}
}
#endif
--
1.8.1.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Commit "drm: run the hpd irq event code directly" causes stutter from repeated EDID retrievals
On Sunday 26 May 2013 18:14:06 Daniel Vetter wrote: > My guess is that you've worked around these edid stalls with > drm_kms_helper.poll=0 and the above commit breaks that w/a. That patch > was shot down unfortunately. If you can't reproduce these stalls on > older kernels when removing the poll=0 option, then something else > broke. You are correct. Before that commit, the stuttering behavior is present with poll=1, but not with poll=0. Starting with that commit, the behavior is present nomatter the poll setting. Am I correct in thinking that this is a problem without a clear solution? - Gard ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm-openchrome status (or will it be in Kernel 3.10?)
> > Dave what is the best way to push this code? I am in no rush so > > I like to do it the right way. We have scattered history in the external > > tree but I would not lose sleep over its lost. I could just post pieces > > of the code to this list for review. One choice is after all the code is > > reviewed is to then merge the branch. Another choice is to create a new > > branch for later merge that will be composed of the patches posted to this > > list. We lose the history but that is not to be of deal for me. Just let > > me know what you would like and I will start the process. > > Hi James, > > I would probably say history loss is inevitable unless the tree is at > a fairly high standard wrt bisection fail etc. > > But it would probably be good to post the driver to dri-devel at least > once so people can take a look, not sure what > the best way to split it up is, the initial patches can be split > different than what we merge if it makes reading it easier. > > Can you post a link to what you consider the best place for me to be > looking so I can see if splitting it makes sense. Except for new pci ids in include/linux/pci_ids.h all the code is here http://cgit.freedesktop.org/openchrome/drm-openchrome/tree/drivers/gpu/drm/via Thank you for taking a look. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 63935] TURKS [drm:r600_uvd_init] *ERROR* UVD not responding, trying to reset the VCPU!!!
https://bugs.freedesktop.org/show_bug.cgi?id=63935 --- Comment #46 from Austin Lund --- The changes didn't make 3.10-rc3 (too soon?). Also, no answers yet for if the test for EFI booted macs it can be removed. -- You are receiving this mail because: You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 64257] RS880 issues with r600-llvm-compiler
https://bugs.freedesktop.org/show_bug.cgi?id=64257 --- Comment #19 from Mike Lothian --- Created attachment 79821 --> https://bugs.freedesktop.org/attachment.cgi?id=79821&action=edit KWin with corruption OK I'm just back and I've recompiled everything from master - unfortunately things have regressed further. I'm not sure if there's colour corruption or not but things do look a little funky -- You are receiving this mail because: You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 50091] [BISECTED]GeForce 6150SE: system hangs on X-server start with garbled screen
https://bugzilla.kernel.org/show_bug.cgi?id=50091 Joachim Namislow changed: What|Removed |Added CC||jfrie...@hotmail.com --- Comment #30 from Joachim Namislow 2013-05-27 05:16:31 --- This is -still- an issue for all kernels >= 3.7. Only kernels up to 3.6.x avoid the locking issue which makes the later ones unusable on systems with this video device, here an on-board nVidia Corporation C61 [GeForce 6100 nForce 405] (rev a2). -- Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 64257] RS880 issues with r600-llvm-compiler
https://bugs.freedesktop.org/show_bug.cgi?id=64257 --- Comment #20 from Marc Dietrich --- similar error with webgl: http://www.gooengine.com/demofiles/pearl-boy/index.html (in firefox). -- You are receiving this mail because: You are the assignee for the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/exynos: fix checks for valid mixer window
From: Krzysztof Kozlowski
Valid values for mixer window are from 0 to MIXER_WIN_NR-1 inclusive.
Arrays in structures (e.g. mixer_context.win_data) have size of
MIXER_WIN_NR so checks for wrong mixer window must be greater-equal.
Signed-off-by: Krzysztof Kozlowski
Signed-off-by: Hyunhee Kim
Signed-off-by: Seung-Woo Kim
---
drivers/gpu/drm/exynos/exynos_drm_hdmi.c |4 ++--
drivers/gpu/drm/exynos/exynos_mixer.c|2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
index 437fb94..b9b2726 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
@@ -344,7 +344,7 @@ static void drm_mixer_commit(struct device *subdrv_dev, int
zpos)
DRM_DEBUG_KMS("%s\n", __FILE__);
- if (win < 0 || win > MIXER_WIN_NR) {
+ if (win < 0 || win >= MIXER_WIN_NR) {
DRM_ERROR("mixer window[%d] is wrong\n", win);
return;
}
@@ -362,7 +362,7 @@ static void drm_mixer_disable(struct device *subdrv_dev,
int zpos)
DRM_DEBUG_KMS("%s\n", __FILE__);
- if (win < 0 || win > MIXER_WIN_NR) {
+ if (win < 0 || win >= MIXER_WIN_NR) {
DRM_ERROR("mixer window[%d] is wrong\n", win);
return;
}
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
b/drivers/gpu/drm/exynos/exynos_mixer.c
index 7c197d38..3658e07 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -742,7 +742,7 @@ static void mixer_win_mode_set(void *ctx,
if (win == DEFAULT_ZPOS)
win = MIXER_DEFAULT_WIN;
- if (win < 0 || win > MIXER_WIN_NR) {
+ if (win < 0 || win >= MIXER_WIN_NR) {
DRM_ERROR("mixer window[%d] is wrong\n", win);
return;
}
--
1.7.4.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: fix checks for valid mixer window
Applied.
Thanks,
Inki Dae
2013/5/27 Seung-Woo Kim
> From: Krzysztof Kozlowski
>
> Valid values for mixer window are from 0 to MIXER_WIN_NR-1 inclusive.
> Arrays in structures (e.g. mixer_context.win_data) have size of
> MIXER_WIN_NR so checks for wrong mixer window must be greater-equal.
>
> Signed-off-by: Krzysztof Kozlowski
> Signed-off-by: Hyunhee Kim
> Signed-off-by: Seung-Woo Kim
> ---
> drivers/gpu/drm/exynos/exynos_drm_hdmi.c |4 ++--
> drivers/gpu/drm/exynos/exynos_mixer.c|2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> index 437fb94..b9b2726 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> @@ -344,7 +344,7 @@ static void drm_mixer_commit(struct device
> *subdrv_dev, int zpos)
>
> DRM_DEBUG_KMS("%s\n", __FILE__);
>
> - if (win < 0 || win > MIXER_WIN_NR) {
> + if (win < 0 || win >= MIXER_WIN_NR) {
> DRM_ERROR("mixer window[%d] is wrong\n", win);
> return;
> }
> @@ -362,7 +362,7 @@ static void drm_mixer_disable(struct device
> *subdrv_dev, int zpos)
>
> DRM_DEBUG_KMS("%s\n", __FILE__);
>
> - if (win < 0 || win > MIXER_WIN_NR) {
> + if (win < 0 || win >= MIXER_WIN_NR) {
> DRM_ERROR("mixer window[%d] is wrong\n", win);
> return;
> }
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
> b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 7c197d38..3658e07 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -742,7 +742,7 @@ static void mixer_win_mode_set(void *ctx,
> if (win == DEFAULT_ZPOS)
> win = MIXER_DEFAULT_WIN;
>
> - if (win < 0 || win > MIXER_WIN_NR) {
> + if (win < 0 || win >= MIXER_WIN_NR) {
> DRM_ERROR("mixer window[%d] is wrong\n", win);
> return;
> }
> --
> 1.7.4.1
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/6] gpu: host1x: Fixes to host1x firewall
On 05/26/2013 01:02 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Fri, May 17, 2013 at 02:49:43PM +0300, Arto Merilainen wrote:
From: Terje Bergstrom
This patch adds several fixes to host1x firewall:
- Host1x firewall does not survive if it expects a reloc, but user
space didn't pass any relocs. Also it reset the reloc table for
each gather, whereas they should be reset only per submit. Also
class does not need to be reset for each class - once per submit
is enough.
- For INCR opcode the check was not working properly at all.
- The firewall verified gather buffers before copying them. This
allowed a malicious application to rewrite the buffer content by
timing the rewrite carefully. This patch makes the buffer
validation occur after copying the buffers.
Can these be split into separate patches, please? It's not only always
good to split logical changes into separate patches but it also makes
reviewing a lot more pleasant. It's hard to tell from this combined
patch which changes belong together.
Sure.
I have a few additional comments inline.
diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index f665d67..4f3c004 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job,
struct host1x_bo *cmdbuf)
void *cmdbuf_page_addr = NULL;
/* pin & patch the relocs for one gather */
- while (i < job->num_relocs) {
+ for (i = 0; i < job->num_relocs; ++i) {
Nit: I prefer post-increment where possible. For consistency.
Will do.
@@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job,
struct host1x_bo *cmdbuf)
return 0;
}
-static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
- unsigned int offset)
+static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
+ unsigned int offset)
{
offset *= sizeof(u32);
- if (reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
- return -EINVAL;
+ if (!reloc || reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
Is the additional !reloc check really necessary? Looking at the callers,
they always pass in fw->relocarray, which in turn is only NULL if no
buffers are to be relocated.
Yes, the additional check is necessary exactly for that reason. The code
will fail if the userspace does not deliver a relocation array and still
pushes data to an address register.
However, the code *should* check that there are relocations left before
even coming here so I probably just fix this differently.
+ return true;
- return 0;
+ return false;
}
I wonder whether we should be changing the logic here and have the
check_reloc() function return true if the relocation is good. I find
that to be more intuitive.
I was also thinking that earlier. Will do.
@@ -508,6 +502,7 @@ int host1x_job_pin(struct host1x_job *job, struct device
*dev)
int err;
unsigned int i, j;
struct host1x *host = dev_get_drvdata(dev->parent);
+
DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
This is an unnecessary whitespace change.
Ops. Will fix.
- Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/6] gpu: host1x: Fix syncpoint wait return value
On 05/26/2013 01:12 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Fri, May 17, 2013 at 02:49:44PM +0300, Arto Merilainen wrote: Syncpoint wait returned EAGAIN if it was called with zero timeout. This patch modifies the function to return ETIMEDOUT. This description is a bit redundant, because it repeats in prose what the code does. I'd rather see a description of why the change is necessary. True. Will fix. Thinking about it, maybe it would be good to have two separate error codes. Keeping -EAGAIN for the case where a zero timeout was passed doesn't sound too bad to differentiate it from the case where a non- zero timeout was passed and it actually timed out. What do you think? I agree, in this case it would not look bad at all. However, user space libraries may loop until the ioctl return code is something else than -EAGAIN or -EINTR. Especially function drmIoctl() in libdrm does this which is why I noted this isssue in the first place. If user space uses zero timeout to just check if a syncpoint value has already passed the library continues looping until the syncpoint value actually passes. Of course, we could just modify the ioctl interface to "cast" this return code to something else but that does not seem correct. - Arto ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/6] gpu: host1x: Fix memory access in syncpt request
On 05/26/2013 01:15 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Fri, May 17, 2013 at 02:49:45PM +0300, Arto Merilainen wrote: This patch fixes a bad memory access in syncpoint request code. If no syncpoints were available, the code accessed unreserved memory area causing unexpected behaviour. Signed-off-by: Arto Merilainen --- drivers/gpu/host1x/syncpt.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c index 5bf5366..6b7ee88 100644 --- a/drivers/gpu/host1x/syncpt.c +++ b/drivers/gpu/host1x/syncpt.c @@ -40,7 +40,7 @@ static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host, for (i = 0; i < host->info->nb_pts && sp->name; i++, sp++) ; - if (sp->dev) + if (i >= host->info->nb_pts) return NULL; While changing this, can you please add a blank line between the loop and the new 'if (...)'? Yep. Will do. - Arto ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 44772] Radeon HD6950 (Cayman): Resuming from hibernation fails sometimes
https://bugs.freedesktop.org/show_bug.cgi?id=44772 Harald Judt changed: What|Removed |Added Status|REOPENED|RESOLVED Resolution|--- |FIXED --- Comment #10 from Harald Judt --- Solved by setting /sys/power/pm_async to 0. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/a6037899/attachment.html>
[PATCH 1/6] gpu: host1x: Fixes to host1x firewall
On Fri, May 17, 2013 at 02:49:43PM +0300, Arto Merilainen wrote:
> From: Terje Bergstrom
>
> This patch adds several fixes to host1x firewall:
> - Host1x firewall does not survive if it expects a reloc, but user
> space didn't pass any relocs. Also it reset the reloc table for
> each gather, whereas they should be reset only per submit. Also
> class does not need to be reset for each class - once per submit
> is enough.
> - For INCR opcode the check was not working properly at all.
> - The firewall verified gather buffers before copying them. This
> allowed a malicious application to rewrite the buffer content by
> timing the rewrite carefully. This patch makes the buffer
> validation occur after copying the buffers.
Can these be split into separate patches, please? It's not only always
good to split logical changes into separate patches but it also makes
reviewing a lot more pleasant. It's hard to tell from this combined
patch which changes belong together.
I have a few additional comments inline.
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index f665d67..4f3c004 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job,
> struct host1x_bo *cmdbuf)
> void *cmdbuf_page_addr = NULL;
>
> /* pin & patch the relocs for one gather */
> - while (i < job->num_relocs) {
> + for (i = 0; i < job->num_relocs; ++i) {
Nit: I prefer post-increment where possible. For consistency.
> @@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job,
> struct host1x_bo *cmdbuf)
> return 0;
> }
>
> -static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
> -unsigned int offset)
> +static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
> + unsigned int offset)
> {
> offset *= sizeof(u32);
>
> - if (reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
> - return -EINVAL;
> + if (!reloc || reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
Is the additional !reloc check really necessary? Looking at the callers,
they always pass in fw->relocarray, which in turn is only NULL if no
buffers are to be relocated.
> + return true;
>
> - return 0;
> + return false;
> }
I wonder whether we should be changing the logic here and have the
check_reloc() function return true if the relocation is good. I find
that to be more intuitive.
> @@ -376,69 +371,58 @@ static int check_nonincr(struct host1x_firewall *fw)
> return 0;
> }
>
> -static int validate(struct host1x_job *job, struct device *dev,
> - struct host1x_job_gather *g)
> +static int validate_gather(struct host1x_firewall *fw,
> +struct host1x_job_gather *g)
I don't think we necessarily need to rename the function. However since
you modify each line that the rename touches anyway it's okay.
> @@ -508,6 +502,7 @@ int host1x_job_pin(struct host1x_job *job, struct device
> *dev)
> int err;
> unsigned int i, j;
> struct host1x *host = dev_get_drvdata(dev->parent);
> +
> DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
This is an unnecessary whitespace change.
Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/348bf507/attachment.pgp>
[PATCH 2/6] gpu: host1x: Fix syncpoint wait return value
On Fri, May 17, 2013 at 02:49:44PM +0300, Arto Merilainen wrote: > Syncpoint wait returned EAGAIN if it was called with zero timeout. > This patch modifies the function to return ETIMEDOUT. This description is a bit redundant, because it repeats in prose what the code does. I'd rather see a description of why the change is necessary. Thinking about it, maybe it would be good to have two separate error codes. Keeping -EAGAIN for the case where a zero timeout was passed doesn't sound too bad to differentiate it from the case where a non- zero timeout was passed and it actually timed out. What do you think? Thierry -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/24d46598/attachment.pgp>
[PATCH 3/6] gpu: host1x: Fix memory access in syncpt request
On Fri, May 17, 2013 at 02:49:45PM +0300, Arto Merilainen wrote: > This patch fixes a bad memory access in syncpoint request code. If > no syncpoints were available, the code accessed unreserved memory > area causing unexpected behaviour. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/syncpt.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c > index 5bf5366..6b7ee88 100644 > --- a/drivers/gpu/host1x/syncpt.c > +++ b/drivers/gpu/host1x/syncpt.c > @@ -40,7 +40,7 @@ static struct host1x_syncpt *_host1x_syncpt_alloc(struct > host1x *host, > > for (i = 0; i < host->info->nb_pts && sp->name; i++, sp++) > ; > - if (sp->dev) > + if (i >= host->info->nb_pts) > return NULL; While changing this, can you please add a blank line between the loop and the new 'if (...)'? Thierry -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/d9949867/attachment.pgp>
[PATCH 4/6] gpu: host1x: Fix client_managed type
On Fri, May 17, 2013 at 02:49:46PM +0300, Arto Merilainen wrote: > client_managed field in syncpoint structure was defined as an > integer. The field holds, however, only a boolean value. This patch > modifies the type to boolean. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/drm/gr2d.c |2 +- > drivers/gpu/host1x/syncpt.c |8 > drivers/gpu/host1x/syncpt.h |6 +++--- > 3 files changed, 8 insertions(+), 8 deletions(-) Looks good. I'll wait for v2 of the series before applying this to make things easier. Thierry -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/14bc2f94/attachment.pgp>
[PATCH 5/6] gpu: host1x: Rework CPU syncpoint increment
On Fri, May 17, 2013 at 02:49:47PM +0300, Arto Merilainen wrote: > This patch merges host1x_syncpt_cpu_incr to host1x_syncpt_incr() as > they are in practise doing the same thing. host1x_syncpt_incr() is also > modified to return error codes. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/dev.h |8 > drivers/gpu/host1x/hw/cdma_hw.c |2 +- > drivers/gpu/host1x/hw/syncpt_hw.c | 12 +--- > drivers/gpu/host1x/syncpt.c | 15 ++- > drivers/gpu/host1x/syncpt.h |7 ++- > 5 files changed, 14 insertions(+), 30 deletions(-) Looks good. Can you carry this over to v2 as well so I can apply all of them in one go? Thierry -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/1f7dd9be/attachment-0001.pgp>
[PATCH 6/6] drm/tegra: Fix syncpoint increment return code
On Fri, May 17, 2013 at 02:49:48PM +0300, Arto Merilainen wrote: > Add syncpoint increment to return a proper return code based on the > return value from host1x. > > Signed-off-by: Arto Merilainen > --- > drivers/gpu/host1x/drm/drm.c |3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Maybe this should be folded into patch 5 because it is related to the fact that host1x_syncpt_incr() now returns proper error codes? Thierry -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/13f84000/attachment.pgp>
Commit "drm: run the hpd irq event code directly" causes stutter from repeated EDID retrievals
On Sun, May 26, 2013 at 5:38 PM, Gard Spreemann wrote: > Hi, > > If I should contact a list instead of you directly, I sincerely apologize! You should have cc'ed relevant lists at least ;-) Fixed up. > After a recent kernel upgrade, I found that my system (using Nouveau) would > stutter whenever eDP-1 was turned off. That display being turned off > apparently > causes repeated EDID retrievals and failed checksums. I did a git bisect which > lead me to > *** > commit 69787f7da6b2adc4054357a661aaa1701a9ca76f > Author: Daniel Vetter > Date: Tue Oct 23 18:23:34 2012 + > > drm: run the hpd irq event code directly > > All drivers already have a work item to run the hpd code, so we don't > need to launch a new one in the helper code. Dave Airlie mentioned > that the cancel+re-queue might paper over DP related hpd ping-pongs, > hence why this is split out. > > Signed-off-by: Daniel Vetter > Reviewed-by: Alex Deucher > Signed-off-by: Dave Airlie > *** > as the first bad commit, and I was advised to contact you as well as file a > bug > with the Nouveau project [1]. > > I don't have enough insight into the code in question to understand the > problem, but your discussion on April 6 [2] seems relevant. > > Please let me know if there's anything I can do to help. > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=64858 > [2] https://patchwork.kernel.org/patch/2402211/ My guess is that you've worked around these edid stalls with drm_kms_helper.poll=0 and the above commit breaks that w/a. That patch was shot down unfortunately. If you can't reproduce these stalls on older kernels when removing the poll=0 option, then something else broke. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Commit "drm: run the hpd irq event code directly" causes stutter from repeated EDID retrievals
On Sun, May 26, 2013 at 6:25 PM, Gard Spreemann wrote: > On Sunday 26 May 2013 18:14:06 Daniel Vetter wrote: >> My guess is that you've worked around these edid stalls with >> drm_kms_helper.poll=0 and the above commit breaks that w/a. That patch >> was shot down unfortunately. If you can't reproduce these stalls on >> older kernels when removing the poll=0 option, then something else >> broke. > > You are correct. Before that commit, the stuttering behavior is present with > poll=1, but not with poll=0. Starting with that commit, the behavior is > present nomatter the poll setting. > > Am I correct in thinking that this is a problem without a clear solution? It's a problem with a solution rejected by Dave and Alex, and I tend to agree with them a bit. The real solution is to grow nouveau/radeon hpd storm mitigation code (i915 already has it in 3.10), since it's much better to cut of the issue at the source (by disasbling hpd interrupts completely for such problematic outputs) instead of trying to work around the worst side-effects of getting too many. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
drm-openchrome status (or will it be in Kernel 3.10?)
> wrote: > > Am Dienstag, 30. April 2013, 06:06:22 schrieb Dave Airlie: > >> On Tue, Apr 30, 2013 at 2:17 AM, Johannes Obermayr > >> wrote: > >> > Hi James, > >> > > >> > Linus recently released Kernel 3.9, merge window for Kernel 3.10 has > >> > been opened, and the question is whether drm-openchrome will be part of > >> > the new Kernel version. > >> > >> Johannes, > >> > >> you misunderstand merge window. The merge window is for stuff to go > >> from toplevel maintainers to Linus, not for new stuff to appear and be > >> merged. > > > > Dave, > > > > I know you maintain also a merge window for drm stuff which starts at ~ rc6. > > > > But I am unsure when it closes: When Linus opens his merge window or when > > you forward your main drm pull request to Linus. > > First case means it is definitely too late for drm-openchrome in 3.10. > > Second case means there can be hope (depending on James' answer) ... > > > > But regarding your answer I assume first case is right. > > Once Linus opens his window, I won't accept anything major that hasn't > been posted to the list for review before. > > I generally have a lag time of hoovering up on the list stuff, but > something like this that has never been posted would have no hope. Hello. Sorry I didn't responded earlier. I was swamped at work with projects and conferences. After I got back from my last conference Xavier informed me about this email and we had a discussion about merging. Currently we have a external tree which is two years old but most of the work has happened in the last 8 months. The reason for delaying mergering was to ensure most distros would be carrying the version of openchrome xorg driver that supports KMS. Today with that barrier gone we have to deal with the drm-openchrome branch not being in top shape. Since I have been back the tree has been cleaned up and the last show stoppers were checked in yesterday. Currently we support LVDS, HDMI/DVI, and VGA outputs which cover most people. By no means is the kernel driver complete but it is usable. I would say the branch is now ready for merger. Dave what is the best way to push this code? I am in no rush so I like to do it the right way. We have scattered history in the external tree but I would not lose sleep over its lost. I could just post pieces of the code to this list for review. One choice is after all the code is reviewed is to then merge the branch. Another choice is to create a new branch for later merge that will be composed of the patches posted to this list. We lose the history but that is not to be of deal for me. Just let me know what you would like and I will start the process.
[PATCH 2/3] drm/gma500: Fix cursor gem obj referencing on psb
The internal crtc cursor gem object pointer was never set/updated since
it was required to be set in the first place.
Fixing this will make the pin/unpin count match and prevent cursor
objects from leaking when userspace drops all references to it. Also
make sure we drop the gem obj reference on failure.
This patch only affects Poulsbo chips.
Signed-off-by: Patrik Jakobsson
---
drivers/gpu/drm/gma500/psb_intel_display.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c
b/drivers/gpu/drm/gma500/psb_intel_display.c
index 6e8f42b..c4a39da 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -843,7 +843,7 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc *crtc,
struct gtt_range *cursor_gt = psb_intel_crtc->cursor_gt;
struct drm_gem_object *obj;
void *tmp_dst, *tmp_src;
- int ret, i, cursor_pages;
+ int ret = 0, i, cursor_pages;
/* if we want to turn of the cursor ignore width and height */
if (!handle) {
@@ -880,7 +880,8 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc *crtc,
if (obj->size < width * height * 4) {
dev_dbg(dev->dev, "buffer is to small\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto unref_cursor;
}
gt = container_of(obj, struct gtt_range, gem);
@@ -889,13 +890,14 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc
*crtc,
ret = psb_gtt_pin(gt);
if (ret) {
dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
- return ret;
+ goto unref_cursor;
}
if (dev_priv->ops->cursor_needs_phys) {
if (cursor_gt == NULL) {
dev_err(dev->dev, "No hardware cursor mem available");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto unref_cursor;
}
/* Prevent overflow */
@@ -936,9 +938,14 @@ static int psb_intel_crtc_cursor_set(struct drm_crtc *crtc,
struct gtt_range, gem);
psb_gtt_unpin(gt);
drm_gem_object_unreference(psb_intel_crtc->cursor_obj);
- psb_intel_crtc->cursor_obj = obj;
}
- return 0;
+
+ psb_intel_crtc->cursor_obj = obj;
+ return ret;
+
+unref_cursor:
+ drm_gem_object_unreference(obj);
+ return ret;
}
static int psb_intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
--
1.8.1.2
[PATCH 3/3] drm/gma500: Fix cursor gem obj referencing on cdv
The internal crtc cursor gem object pointer was never set/updated since
it was required to be set in the first place.
Fixing this will make the pin/unpin count match and prevent cursor
objects from leaking when userspace drops all references to it. Also
make sure we drop the gem obj reference on failure.
This patch only affects Cedarview chips.
Signed-off-by: Patrik Jakobsson
---
drivers/gpu/drm/gma500/cdv_intel_display.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c
b/drivers/gpu/drm/gma500/cdv_intel_display.c
index 3cfd093..f30d00f 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_display.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_display.c
@@ -1462,7 +1462,7 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
size_t addr = 0;
struct gtt_range *gt;
struct drm_gem_object *obj;
- int ret;
+ int ret = 0;
/* if we want to turn of the cursor ignore width and height */
if (!handle) {
@@ -1499,7 +1499,8 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
if (obj->size < width * height * 4) {
dev_dbg(dev->dev, "buffer is to small\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto unref_cursor;
}
gt = container_of(obj, struct gtt_range, gem);
@@ -1508,7 +1509,7 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
ret = psb_gtt_pin(gt);
if (ret) {
dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle);
- return ret;
+ goto unref_cursor;
}
addr = gt->offset; /* Or resource.start ??? */
@@ -1532,9 +1533,14 @@ static int cdv_intel_crtc_cursor_set(struct drm_crtc
*crtc,
struct gtt_range, gem);
psb_gtt_unpin(gt);
drm_gem_object_unreference(psb_intel_crtc->cursor_obj);
- psb_intel_crtc->cursor_obj = obj;
}
- return 0;
+
+ psb_intel_crtc->cursor_obj = obj;
+ return ret;
+
+unref_cursor:
+ drm_gem_object_unreference(obj);
+ return ret;
}
static int cdv_intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
--
1.8.1.2
[PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
A framebuffer is pinned when it's set as pipe base, so we also need to unpin it when it's destroyed. Some framebuffers are never used as scanout (no mode set on them) so if the pin count is less than one, no unpinning is needed. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 Signed-off-by: Patrik Jakobsson --- drivers/gpu/drm/gma500/framebuffer.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 8b1b6d9..1a11b69 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct drm_framebuffer *fb) /* Let DRM do its clean up */ drm_framebuffer_cleanup(fb); + + /* Unpin any psb_intel_pipe_set_base() pinning */ + if (r->in_gart > 0) + psb_gtt_unpin(r); + /* We are no longer using the resource in GEM */ drm_gem_object_unreference_unlocked(&r->gem); kfree(fb); -- 1.8.1.2
[PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
On Sun, May 26, 2013 at 08:03:53PM +0200, Patrik Jakobsson wrote: > A framebuffer is pinned when it's set as pipe base, so we also need to > unpin it when it's destroyed. Some framebuffers are never used as > scanout (no mode set on them) so if the pin count is less than one, no > unpinning is needed. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 > Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 > > Signed-off-by: Patrik Jakobsson > --- > drivers/gpu/drm/gma500/framebuffer.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/gma500/framebuffer.c > b/drivers/gpu/drm/gma500/framebuffer.c > index 8b1b6d9..1a11b69 100644 > --- a/drivers/gpu/drm/gma500/framebuffer.c > +++ b/drivers/gpu/drm/gma500/framebuffer.c > @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct > drm_framebuffer *fb) > > /* Let DRM do its clean up */ > drm_framebuffer_cleanup(fb); > + > + /* Unpin any psb_intel_pipe_set_base() pinning */ > + if (r->in_gart > 0) > + psb_gtt_unpin(r); The drm core /should/ have removed the user framebuffer from all active users before it calls down into your ->destroy callback. Why can't gma500 just pin/unpin on demand when the buffer is actually in use? If a pin survives the official use as seen by the drm core (e.g. to keep a buffer around until the pageflip completes) you can simply keep an additional reference around. Cheers, Daniel > + > /* We are no longer using the resource in GEM */ > drm_gem_object_unreference_unlocked(&r->gem); > kfree(fb); > -- > 1.8.1.2 > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter wrote: > On Sun, May 26, 2013 at 08:03:53PM +0200, Patrik Jakobsson wrote: >> A framebuffer is pinned when it's set as pipe base, so we also need to >> unpin it when it's destroyed. Some framebuffers are never used as >> scanout (no mode set on them) so if the pin count is less than one, no >> unpinning is needed. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 >> Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 >> >> Signed-off-by: Patrik Jakobsson >> --- >> drivers/gpu/drm/gma500/framebuffer.c | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/gma500/framebuffer.c >> b/drivers/gpu/drm/gma500/framebuffer.c >> index 8b1b6d9..1a11b69 100644 >> --- a/drivers/gpu/drm/gma500/framebuffer.c >> +++ b/drivers/gpu/drm/gma500/framebuffer.c >> @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct >> drm_framebuffer *fb) >> >> /* Let DRM do its clean up */ >> drm_framebuffer_cleanup(fb); >> + >> + /* Unpin any psb_intel_pipe_set_base() pinning */ >> + if (r->in_gart > 0) >> + psb_gtt_unpin(r); > > The drm core /should/ have removed the user framebuffer from all active > users before it calls down into your ->destroy callback. Why can't gma500 > just pin/unpin on demand when the buffer is actually in use? Thanks for the feedback DRM core does correctly keep track of the users but the last ref is dropped in our ->destroy callback and at that point it's still pinned from pipe_set_base(). So if it's considered too late to unpin the framebuffer in destroy, we never had it right in the first place. Not a big surprise I guess :) > If a pin survives the official use as seen by the drm core (e.g. to keep a > buffer around until the pageflip completes) you can simply keep an > additional reference around. As I wrote above, that additional reference is not dropped until ->destroy anyways and we don't have page flipping support and to be honest I haven't even looked at implementing it yet. So the logical point to release the pin would be in pipe_set_base() (or am I wrong?) by doing an unpin on the old_fb. Problem is that when killing X and switching back to fbdev we get no old_fb. I might be missing something, so any suggestions are welcome. Thanks Patrik
[Bug 65016] New: [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 Priority: medium Bug ID: 65016 Assignee: dri-devel at lists.freedesktop.org Summary: [radeonsi] X server segfaults at startup with mesa-9.1* Severity: normal Classification: Unclassified OS: All Reporter: alexander at tsoy.me Hardware: Other Status: NEW Version: 9.1 Component: Drivers/Gallium/radeonsi Product: Mesa X server segfaults at startup whith mesa 9.1*. No problems with Git snapshots of mesa (these are the only versions of mesa which works on my system). I'll attach two backtraces below: 1. llvm and mesa was compiled with debugging support. Unfortunately this trace is incomplete =/ 2. llvm was compiled without debugging support, mesa was compiled whith debugging support. This trace is complete, but lacks a lot of debugging info. Hardware: 01:00.0 VGA compatible controller: Advanced Micro Devices [AMD] nee ATI Cape Verde PRO [Radeon HD 7750] Subsystem: Giga-byte Technology Device 2260 Kernel driver in use: radeon Kernel modules: radeon Software: - mesa 9.1.3, also tried earlier minor versions - llvm 3.3_rc2, llvm Git - xorg-server-1.13.4 - xf86-video-ati-7.1.0 - glamor Git - linux 3.8.12 -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/4f440d37/attachment.html>
[Bug 65016] [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 --- Comment #1 from Alexander Tsoy --- Created attachment 79818 --> https://bugs.freedesktop.org/attachment.cgi?id=79818&action=edit [1] - both llvm and mesa with debugging support -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/4b2ce04d/attachment.html>
[Bug 65016] [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 --- Comment #2 from Alexander Tsoy --- Created attachment 79819 --> https://bugs.freedesktop.org/attachment.cgi?id=79819&action=edit backtrace [2] - only mesa with debugging support -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/751a640a/attachment.html>
[Bug 65016] [radeonsi] X server segfaults at startup with mesa-9.1*
https://bugs.freedesktop.org/show_bug.cgi?id=65016 Alexander Tsoy changed: What|Removed |Added Attachment #79818|[1] - both llvm and mesa|backtrace [1] - both llvm description|with debugging support |and mesa with debugging ||support -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/28f05fd3/attachment.html>
[PATCH 1/3] drm/gma500: Unpin framebuffer when destroying it
On Sun, May 26, 2013 at 10:31 PM, Patrik Jakobsson wrote: > On Sun, May 26, 2013 at 9:07 PM, Daniel Vetter wrote: >> On Sun, May 26, 2013 at 08:03:53PM +0200, Patrik Jakobsson wrote: >>> A framebuffer is pinned when it's set as pipe base, so we also need to >>> unpin it when it's destroyed. Some framebuffers are never used as >>> scanout (no mode set on them) so if the pin count is less than one, no >>> unpinning is needed. >>> >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=889511 >>> Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=812113 >>> >>> Signed-off-by: Patrik Jakobsson >>> --- >>> drivers/gpu/drm/gma500/framebuffer.c | 5 + >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/gma500/framebuffer.c >>> b/drivers/gpu/drm/gma500/framebuffer.c >>> index 8b1b6d9..1a11b69 100644 >>> --- a/drivers/gpu/drm/gma500/framebuffer.c >>> +++ b/drivers/gpu/drm/gma500/framebuffer.c >>> @@ -668,6 +668,11 @@ static void psb_user_framebuffer_destroy(struct >>> drm_framebuffer *fb) >>> >>> /* Let DRM do its clean up */ >>> drm_framebuffer_cleanup(fb); >>> + >>> + /* Unpin any psb_intel_pipe_set_base() pinning */ >>> + if (r->in_gart > 0) >>> + psb_gtt_unpin(r); >> >> The drm core /should/ have removed the user framebuffer from all active >> users before it calls down into your ->destroy callback. Why can't gma500 >> just pin/unpin on demand when the buffer is actually in use? > > Thanks for the feedback > > DRM core does correctly keep track of the users but the last ref is dropped in > our ->destroy callback and at that point it's still pinned from > pipe_set_base(). > So if it's considered too late to unpin the framebuffer in destroy, we never > had > it right in the first place. Not a big surprise I guess :) > >> If a pin survives the official use as seen by the drm core (e.g. to keep a >> buffer around until the pageflip completes) you can simply keep an >> additional reference around. > > As I wrote above, that additional reference is not dropped until ->destroy > anyways and we don't have page flipping support and to be honest I haven't > even > looked at implementing it yet. So the logical point to release the pin would > be > in pipe_set_base() (or am I wrong?) by doing an unpin on the old_fb. Problem > is > that when killing X and switching back to fbdev we get no old_fb. > > I might be missing something, so any suggestions are welcome. Dumping our irc discussion for google to index here: I sounds like gma500 code is missing the crtc disabling sequence when X shuts down, i.e. the crtc on->off transition. Then when you switch to fbcon you only get a crtc off->on state transition and so don't see an old fb in set_base, which means that the pin refcount of the old framebuffer is lost. To fix this you can use the special call to crtc_funcs->disable (instead of the default crtc_funcs->dpms(OFF)) in drm_helper_disable_unused_functions. Note that X could also do the vt switch first and then only do the framebuffer destruction, in which case I think your patch here would drop the pin reference twice: Once in set_base since we have an old_fb and once in the framebuffer destroy callback. See intel_crtc_disable in intel_display.c in a v3.6 kernel version for how drm/i915 cleanup up the fb pin reference before we've reworked our modeset code and switched away from the crtc helpers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[REGRESSION] system does not resume from ram due to commit "drm/nv50/fifo: prevent races between clients updating playlists"
My NV96 does not resume from suspend to ram (the screen stays black, magic sysrq keys do work) with the current linus git kernel, i bisected it to the following commit. drm/nv50/fifo: prevent races between clients updating playlists b5096566f6e1ee2b88324772f020ae9bc0cfa9a0 It's not obvious to me how this causes problems, but reverting this commit does solve my problem. -- Far away from the primal instinct, the song seems to fade away, the river get wider between your thoughts and the things we do and say. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/b844ec7e/attachment.html>
[PATCH v3, part2 14/20] PCI, DRM: use hotplug-safe iterators to walk PCI buses
Enhance DRM drvier to use hotplug-safe iterators to walk PCI buses.
Signed-off-by: Jiang Liu
Cc: David Airlie
Cc: dri-devel at lists.freedesktop.org
Cc: linux-kernel at vger.kernel.org
---
drivers/gpu/drm/drm_fops.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 429e07d..dc8c40b 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -359,9 +359,11 @@ static int drm_open_helper(struct inode *inode, struct
file *filp,
pci_dev_put(pci_dev);
}
if (!dev->hose) {
- struct pci_bus *b = pci_bus_b(pci_root_buses.next);
- if (b)
+ struct pci_bus *b = pci_get_next_root_bus(NULL);
+ if (b) {
dev->hose = b->sysdata;
+ pci_bus_put(b);
+ }
}
}
#endif
--
1.8.1.2
Commit "drm: run the hpd irq event code directly" causes stutter from repeated EDID retrievals
On Sunday 26 May 2013 18:14:06 Daniel Vetter wrote: > My guess is that you've worked around these edid stalls with > drm_kms_helper.poll=0 and the above commit breaks that w/a. That patch > was shot down unfortunately. If you can't reproduce these stalls on > older kernels when removing the poll=0 option, then something else > broke. You are correct. Before that commit, the stuttering behavior is present with poll=1, but not with poll=0. Starting with that commit, the behavior is present nomatter the poll setting. Am I correct in thinking that this is a problem without a clear solution? - Gard
drm-openchrome status (or will it be in Kernel 3.10?)
> > Dave what is the best way to push this code? I am in no rush so > > I like to do it the right way. We have scattered history in the external > > tree but I would not lose sleep over its lost. I could just post pieces > > of the code to this list for review. One choice is after all the code is > > reviewed is to then merge the branch. Another choice is to create a new > > branch for later merge that will be composed of the patches posted to this > > list. We lose the history but that is not to be of deal for me. Just let > > me know what you would like and I will start the process. > > Hi James, > > I would probably say history loss is inevitable unless the tree is at > a fairly high standard wrt bisection fail etc. > > But it would probably be good to post the driver to dri-devel at least > once so people can take a look, not sure what > the best way to split it up is, the initial patches can be split > different than what we merge if it makes reading it easier. > > Can you post a link to what you consider the best place for me to be > looking so I can see if splitting it makes sense. Except for new pci ids in include/linux/pci_ids.h all the code is here http://cgit.freedesktop.org/openchrome/drm-openchrome/tree/drivers/gpu/drm/via Thank you for taking a look.
[Bug 63935] TURKS [drm:r600_uvd_init] *ERROR* UVD not responding, trying to reset the VCPU!!!
https://bugs.freedesktop.org/show_bug.cgi?id=63935 --- Comment #46 from Austin Lund --- The changes didn't make 3.10-rc3 (too soon?). Also, no answers yet for if the test for EFI booted macs it can be removed. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/6aa88349/attachment.html>
[Bug 64257] RS880 issues with r600-llvm-compiler
https://bugs.freedesktop.org/show_bug.cgi?id=64257 --- Comment #19 from Mike Lothian --- Created attachment 79821 --> https://bugs.freedesktop.org/attachment.cgi?id=79821&action=edit KWin with corruption OK I'm just back and I've recompiled everything from master - unfortunately things have regressed further. I'm not sure if there's colour corruption or not but things do look a little funky -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130526/3ab40b83/attachment.html>
