Re: [Freedreno] [PATCH 07/25] drm/msm/dpu: reserve using crtc state
On 2018-10-09 23:28, Jeykumar Sankaran wrote:
On 2018-10-09 14:06, Sean Paul wrote:
On Mon, Oct 08, 2018 at 09:27:24PM -0700, Jeykumar Sankaran wrote:
DPU maintained reservation lists to cache assigned
HW blocks for the display and a retrieval mechanism for
the individual DRM components to query their respective
HW blocks.
This patch uses the sub-classed CRTC state to store
and track HW blocks assigned for different components
of the display pipeline. It helps the driver:
- to get rid of unwanted store and retrieval RM API's
- to preserve HW resources assigned in atomic_check
through atomic swap/duplicate.
Separate patch is submitted to remove resource
reservation in atomic_commit path.
Signed-off-by: Jeykumar Sankaran
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 65
+++---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 14 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 28 +++---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 20 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 58
---
5 files changed, 72 insertions(+), 113 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4960641..0625f56 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -421,69 +421,20 @@ void dpu_crtc_complete_commit(struct drm_crtc
*crtc,
trace_dpu_crtc_complete_commit(DRMID(crtc));
}
-static void _dpu_crtc_setup_mixer_for_encoder(
- struct drm_crtc *crtc,
- struct drm_encoder *enc)
+static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
{
struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
- struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
- struct dpu_rm *rm = _kms->rm;
struct dpu_crtc_mixer *mixer;
- struct dpu_hw_ctl *last_valid_ctl = NULL;
- int i;
- struct dpu_rm_hw_iter lm_iter, ctl_iter;
-
- dpu_rm_init_hw_iter(_iter, enc->base.id, DPU_HW_BLK_LM);
- dpu_rm_init_hw_iter(_iter, enc->base.id, DPU_HW_BLK_CTL);
+ int i, ctl_index;
/* Set up all the mixers and ctls reserved by this encoder */
- for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++)
{
+ for (i = 0; i < cstate->num_mixers; i++) {
mixer = >mixers[i];
- if (!dpu_rm_get_hw(rm, _iter))
- break;
- mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw;
-
/* CTL may be <= LMs, if <, multiple LMs controlled by 1
CTL */
- if (!dpu_rm_get_hw(rm, _iter)) {
- DPU_DEBUG("no ctl assigned to lm %d, using
previous\n",
- mixer->hw_lm->idx - LM_0);
- mixer->lm_ctl = last_valid_ctl;
- } else {
- mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
- last_valid_ctl = mixer->lm_ctl;
- }
-
- /* Shouldn't happen, mixers are always >= ctls */
- if (!mixer->lm_ctl) {
- DPU_ERROR("no valid ctls found for lm %d\n",
- mixer->hw_lm->idx - LM_0);
- return;
- }
-
- cstate->num_mixers++;
- DPU_DEBUG("setup mixer %d: lm %d\n",
- i, mixer->hw_lm->idx - LM_0);
- DPU_DEBUG("setup mixer %d: ctl %d\n",
- i, mixer->lm_ctl->idx - CTL_0);
- }
-}
-
-static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
-{
- struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
- struct drm_encoder *enc;
-
- mutex_lock(_crtc->crtc_lock);
- /* Check for mixers on all encoders attached to this crtc */
- list_for_each_entry(enc, >dev->mode_config.encoder_list,
head) {
- if (enc->crtc != crtc)
- continue;
-
- _dpu_crtc_setup_mixer_for_encoder(crtc, enc);
+ ctl_index = min(i, cstate->num_ctls - 1);
This is another one of those places I mentioned where we're just
assuming
a
value is going to be in a certain range. If
num_ctls/num_intfs/num_phys_encs
(all the same value afaict) is 0, we end up in a bad place.
Even though all these variables have the same value, they are
representing the
sizes of logically seperate components.
At a minimum, there should be a WARN_ON/BUG_ON somewhere ensuring this
can
never
drop below 0.
Isn't RM guaranteeing that? I can add the WARN_ON checks on these
num_xxx when the HW blocks are allocated.
Thanks,
Jeykumar S.
+ mixer->lm_ctl = cstate->hw_ctls[ctl_index];
}
-
- mutex_unlock(_crtc->crtc_lock);
}
static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
@@ -536,10 +487,8 @@ static void dpu_crtc_atomic_begin(struct
Re: [Freedreno] [PATCH v2 3/3] dt-bindings: msm/disp: Introduce interconnect bindings for MDSS on SDM845
On Wed, Oct 10, 2018 at 02:54:34PM +0530, Sravanthi Kollukuduru wrote: > Add interconnect properties such as interconnect provider specifier > , the edge source and destination ports which are required by the > interconnect API to configure interconnect path for MDSS. > > Changes in v2: > -none > > Signed-off-by: Sravanthi Kollukuduru > --- > Documentation/devicetree/bindings/display/msm/dpu.txt | 8 > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt > b/Documentation/devicetree/bindings/display/msm/dpu.txt > index ad2e8830324e..abd4d99b5030 100644 > --- a/Documentation/devicetree/bindings/display/msm/dpu.txt > +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt > @@ -28,6 +28,11 @@ Required properties: > - #address-cells: number of address cells for the MDSS children. Should be 1. > - #size-cells: Should be 1. > - ranges: parent bus address space is the same as the child bus address > space. > +- interconnects : pairs of phandles and interconnect provider specifier to > + denote the edge source and destination ports of the interconnect path. > +- interconnect-names : list of interconnect path name strings sorted in the > + same order as the interconnects property. Consumers drivers will use > + interconnect-names to match interconnect paths with interconnect > specifiers. Don't need to define common properties. Just list their constraints such as how many. > > Optional properties: > - assigned-clocks: list of clock specifiers for clocks needing rate > assignment > @@ -86,6 +91,9 @@ Example: > interrupt-controller; > #interrupt-cells = <1>; > > + interconnects = < 38 512>; > + interconnect-names = "port0"; -names is kind of pointless here. IMO, interconnect-names should describe the path. > + > iommus = <_iommu 0>; > > #address-cells = <2>; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/3] lib/string: Pass the input gfp flags to kmalloc
Thanks, I've pushed 2 and 3 to msm-next, since these should probably
go in a -fixes pr for 4.20
This one, you might want to resend w/
--cc-cmd=./scripts/get_maintainer.pl so that it gets seen by someone
who could apply it
fwiw, I use
git config sendemail.cccmd './scripts/get_maintainer.pl -i'
to make that automatic, but to give me a way to edit the list of cc's
that get_maintainer.pl adds
BR,
-R
On Fri, Oct 12, 2018 at 4:57 AM Sharat Masetty wrote:
>
> Pass the user sent gfp flags to kmalloc() calls. This helps calling the
> functions in user desired contexts.
>
> Signed-off-by: Sharat Masetty
> ---
> lib/string_helpers.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 29c490e..60f9015 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -576,7 +576,7 @@ char *kstrdup_quotable_cmdline(struct task_struct *task,
> gfp_t gfp)
> char *buffer, *quoted;
> int i, res;
>
> - buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + buffer = kmalloc(PAGE_SIZE, gfp);
> if (!buffer)
> return NULL;
>
> @@ -612,7 +612,7 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp)
> return kstrdup("", gfp);
>
> /* We add 11 spaces for ' (deleted)' to be appended */
> - temp = kmalloc(PATH_MAX + 11, GFP_KERNEL);
> + temp = kmalloc(PATH_MAX + 11, gfp);
> if (!temp)
> return kstrdup("", gfp);
>
> --
> 1.9.1
>
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 08/13] drm/msm/gpu: Rearrange the code that collects the task during a hang
On 8/4/2018 10:47 PM, Rob Clark wrote:
On Thu, Jul 12, 2018 at 3:48 PM Chris Wilson wrote:
Quoting Jordan Crouse (2018-07-12 19:59:25)
Do a bit of cleanup to prepare for upcoming changes to pass the
hanging task comm and cmdline to the crash dump function.
Signed-off-by: Jordan Crouse
---
drivers/gpu/drm/msm/msm_gpu.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 1c09acfb4028..2ca354047250 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -314,6 +314,7 @@ static void recover_worker(struct work_struct *work)
struct msm_drm_private *priv = dev->dev_private;
struct msm_gem_submit *submit;
struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu);
+ char *comm = NULL, *cmd = NULL;
int i;
mutex_lock(>struct_mutex);
@@ -327,7 +328,7 @@ static void recover_worker(struct work_struct *work)
rcu_read_lock();
task = pid_task(submit->pid, PIDTYPE_PID);
if (task) {
- char *cmd;
+ comm = kstrdup(task->comm, GFP_KERNEL);
Under rcu_read_lock(), GFP_KERNEL is not allowed, you need GFP_NOWAIT or
some such (or grab a reference to the pid and drop rcu then GFP_KERNEL).
I started looking at a similar issue w/ our use of
kstrdup_quotable_cmdline() under rcu_read_lock().. I *guess* I hadn't
noticed it before due to different RCU kconfig?
I can use GFP_ATOMIC, and I can fix kstrdup_quotable_cmdline() to
actually use gfp flags passed in for kmalloc() (and similar bug in
kstrdup_quotable_file()).. but get_cmdline() still grabs mmap_sem
which complains under rcu_read_lock()..
is there any way to ensure the tast_struct sticks around long enough
to get it's cmdline without holding rcu_read_lock()? I couldn't find
any refcnt'ing on task_struct itself, which makes this seem a bit
unsolveable :-/
I have been seeing similar issues on my downstream setup and was looking
into fixing this actively. Here is a way to have the task stay afloat
and revert to GFP_KERNEL
https://patchwork.freedesktop.org/patch/256397/ Please review...
I tried this out and it does work.
BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 2/3] drm/msm: Check if target supports crash dump capture
This patch simply checks first to see if the target can support crash dump
capture before proceeding.
Signed-off-by: Sharat Masetty
---
drivers/gpu/drm/msm/msm_gpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 19b4afe..da63d3d 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -345,6 +345,10 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
{
struct msm_gpu_state *state;
+ /* Check if the target supports capturing crash state */
+ if (!gpu->funcs->gpu_state_get)
+ return;
+
/* Only save one crash state at a time */
if (gpu->crashstate)
return;
--
1.9.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 0/3] drm/msm: Fix gpu recovery path
A few fixes for issues that I have seen in recovery path. The fix in lib/string is probably not needed anymore with the fixes in drm/msm, but I added it here nonetheless as it is good to have. Sharat Masetty (3): lib/string: Pass the input gfp flags to kmalloc drm/msm: Check if target supports crash dump capture drm/msm: Fix task dump in gpu recovery drivers/gpu/drm/msm/msm_gpu.c | 13 - lib/string_helpers.c | 4 ++-- 2 files changed, 10 insertions(+), 7 deletions(-) -- 1.9.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 1/3] lib/string: Pass the input gfp flags to kmalloc
Pass the user sent gfp flags to kmalloc() calls. This helps calling the
functions in user desired contexts.
Signed-off-by: Sharat Masetty
---
lib/string_helpers.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 29c490e..60f9015 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -576,7 +576,7 @@ char *kstrdup_quotable_cmdline(struct task_struct *task,
gfp_t gfp)
char *buffer, *quoted;
int i, res;
- buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ buffer = kmalloc(PAGE_SIZE, gfp);
if (!buffer)
return NULL;
@@ -612,7 +612,7 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp)
return kstrdup("", gfp);
/* We add 11 spaces for ' (deleted)' to be appended */
- temp = kmalloc(PATH_MAX + 11, GFP_KERNEL);
+ temp = kmalloc(PATH_MAX + 11, gfp);
if (!temp)
return kstrdup("", gfp);
--
1.9.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 3/3] drm/msm: Fix task dump in gpu recovery
The current recovery code gets a pointer to the task struct and does a
few things all within the rcu_read_lock. This puts constraints on the
types of gfp flags that can be used within the rcu lock. This patch
instead gets a reference to the task within the rcu lock and releases
the lock immediately, this way the task stays afloat until we need it and
we also get to use the desired gfp flags.
Signed-off-by: Sharat Masetty
---
drivers/gpu/drm/msm/msm_gpu.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index da63d3d..ca573f6 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -438,10 +438,9 @@ static void recover_worker(struct work_struct *work)
if (submit) {
struct task_struct *task;
- rcu_read_lock();
- task = pid_task(submit->pid, PIDTYPE_PID);
+ task = get_pid_task(submit->pid, PIDTYPE_PID);
if (task) {
- comm = kstrdup(task->comm, GFP_ATOMIC);
+ comm = kstrdup(task->comm, GFP_KERNEL);
/*
* So slightly annoying, in other paths like
@@ -454,10 +453,10 @@ static void recover_worker(struct work_struct *work)
* about the submit going away.
*/
mutex_unlock(>struct_mutex);
- cmd = kstrdup_quotable_cmdline(task, GFP_ATOMIC);
+ cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+ put_task_struct(task);
mutex_lock(>struct_mutex);
}
- rcu_read_unlock();
if (comm && cmd) {
dev_err(dev->dev, "%s: offending task: %s (%s)\n",
--
1.9.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
