Re: [Freedreno] [RFC 0/4] freedreno: Move some compiler offset computations to NIR
On 1/26/19 12:42 AM, Rob Clark wrote: > On Fri, Jan 25, 2019 at 10:48 AM Eduardo Lima Mitev wrote: >> >> There are a bunch of instructions emitted on ir3_compiler_nir related to >> offset computations for IO opcodes (ssbo, image, etc). This small series >> explores the possibility of moving these instructions to NIR, where we >> have higher chances of optimizing them. >> >> The series introduces a new, freedreno specific NIR pass, >> 'ir3_nir_lower_sampler_io' (final name not set). The pass is executed >> early on ir3_optimize_nir(), and the goal is to centralize all these >> computations there, hoping that later NIR passes will produce better >> code than what is currently emitted. > > I can think of a few other things that would be interesting to lower > to driver specific nir opcodes (imul and various lowering for tex > instructions come to mind.. but probably also ubo and ssbo address > calculation.. maybe it could even make sense for some of the single > src alu instructions that translate into multiple ir3 instructions, > not sure).. > Yes, the plan is to abstract to NIR whatever brings us a benefit in instruction stats. There is also the question of just simplifying the backend compiler, provided we don't hurt produced code. > Are you thinking about having separate passes for each? I guess at > least for alu instructions we might able to use nir_algebraic so > having things split up might be easier. > I haven't thought too much about this yet, but it seems to make sense having at least 2 passes, one for I/O and one for ALUs. >> So far, we have just implemented byte-offset computation for image store >> and atomics. This seemed like a good first target given the amount of >> instructions being emitted for it by the backend. >> >> This is an RFC series because there are a few open questions, but we >> wanted to gather feedback already now, in case this effort is something >> not worth it; and also hoping that somebody else will give it a try >> against other shaders and on other gens (we have just tried this on >> a5xx). >> >> * We have so far been unable to see any improvement in generated code >> (not a penalty either). shader-db has not been specially useful. Few >> shaders there exercise image store or image atomic ops, and of those >> that do, most require higher versions of GLSL than what freedreno >> supports, so they get skipped. The few that do actually run, don't >> show any meaningful difference. > > I guess it would be easy enough to construct shaders that would > benefit from this, but maybe that is cheating.. > > Possibly UBO and SSBO is a better target, I guess there you might be > more likely to see patterns of access of successive elements (ie. > foo[idx], foo[idx+1], etc)? > I took a first stab at SSBO's load/store/atomic, where the offset is divided by 4 in the backend, but was bitten by IR3_STGB requiring both the original byte-offset and the dword-offset (in src1 and src2 respectively). So trivially emitting a nir_shr on the offset didn't buy us anything. I have in the backlog to revisit this, turning the offset into a 2-component reg so we can hold the original byte-offset and the offset divided by 4. > Anyways, since we don't try to do (and I'd rather not do) any sort of > CSE post nir->ir3 I think starting to introduce more ir3 specific > nir->nir lowering seems like a thing we need, so I'm pretty happy that > someone is looking at this :-) > Thanks, that's encouraging. Lets see how far we can get :). Eduardo > BR, > -R > >> Then other shaders picked from tests suites are simple enough not to >> produce any difference in code either. >> >> There is still on-going work looking for cases where the pass helps >> instruction stats, whether writing custom shaders or porting complex >> shader from shader-db to run on GLES 310. >> >> There is though an open question here as to whether moving backend >> code to NIR is a benefit in and of itself. >> >> * The series adds a nir_op_imad opcode that didn't exist before, and >> perhaps not generally useful even for freedreno outside this pass, >> because it maps to IR3_MAD_S24 which is probably not suitable for >> generic integer multiply-add. >> >> * The pass currently has 2 alternative code-paths to emit the >> multiplication by the bytes-per-pixel of an image format. In one >> case, since this value can be obtained at compile time, it is >> emitted as an immediate by nir_imul_imm. The other alternative is >> emitting an nir_imul with an SSA value that will map to >> image_dims[0] at shader runtime. >> >> The former case is uglier but produces better code (a single SHL >> instruction), whereas the latter involves a generic imul, for which >> the backend emits a lot of code to cover for overflow. >> >> The doubt here is whether we should introduce a (lower precision) >> version of imul that maps directly to IR3_IMUL_S. >> >> >> A live (WIP) tree of the series can be found at: >>
Re: [Freedreno] [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
On 1/26/19 5:34 PM, Jason Ekstrand wrote: > Mind suffixing it with _ir3 or something since it's a back-end-specific > intrinsic? Incidentally, this looks a lot like load_image_param_intel. > Yes, I felt inclined to add the suffix but wasn't sure how good/bad a practice it is, so I deferred it to review :). Now, I did a quick experiment and it turns out I can reuse image_deref_load_param_intel as-is. The semantics are a bit different so I would have to fork the comment to explain ir3 too. So, what about I remove the '_intel' suffix to that one and use if for this ir3? Thanks for the feedback and the tip! Eduardo > On January 25, 2019 07:48:54 Eduardo Lima Mitev wrote: > >> This is an internal intrinsic intended to be injected by a >> (freedreno-specific) 'lower_sampler_io' pass that will be introduced >> later in this series; and consumed by ir3_compiler_nir. >> >> The intrinsic will load in SSA values for various constants >> for images (image_dims), namely the format's bytes-per-pixel, >> y-stride and z-stride; for which the backend compiler will emit >> the corresponding uniforms. >> >> const_index[0] is the image index, and const_index[1] is the index >> into image_dims' register file for a given image: 0 for bpp, 1 to >> y-stride and 2 for z-stride. >> --- >> src/compiler/nir/nir_intrinsics.py | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/compiler/nir/nir_intrinsics.py >> b/src/compiler/nir/nir_intrinsics.py >> index a5cc3f7401c..169c835d746 100644 >> --- a/src/compiler/nir/nir_intrinsics.py >> +++ b/src/compiler/nir/nir_intrinsics.py >> @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET], >> [CAN_ELIMINATE]) >> load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) >> # src[] = { offset }. const_index[] = { base, range } >> load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) >> +# src[] = { offset }. const_index[] = { image_idx, dim_idx } >> +load("image_stride", 1, [], [CAN_REORDER]) >> >> # Stores work the same way as loads, except now the first source is >> the value >> # to store and the second (and possibly third) source specify where to >> store >> -- >> 2.20.1 >> >> ___ >> mesa-dev mailing list >> mesa-...@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
On 1/28/19 9:56 AM, Bas Nieuwenhuizen wrote: > On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev wrote: >> >> On 1/26/19 5:34 PM, Jason Ekstrand wrote: >>> Mind suffixing it with _ir3 or something since it's a back-end-specific >>> intrinsic? Incidentally, this looks a lot like load_image_param_intel. >>> >> >> Yes, I felt inclined to add the suffix but wasn't sure how good/bad a >> practice it is, so I deferred it to review :). >> >> Now, I did a quick experiment and it turns out I can reuse >> image_deref_load_param_intel as-is. The semantics are a bit different so >> I would have to fork the comment to explain ir3 too. >> >> So, what about I remove the '_intel' suffix to that one and use if for >> this ir3? > > Instructions that have different semantics for different drivers sound > like a lot of potential for confusion and latent bugs to me. Is it > really a problem to have both? > It is not a problem at all, but I think it is preferable not to duplicate this intrinsic if it serves the same purpose on both backends, and only slightly differ in the specific set of params it holds: On both backends the intrinsic is used to represent registers at compile time, that will be resolved to uniforms at shader run time. On intel, the params are offset, tiling, stride and swizzling; on ir3 they are bytes-per-pixel, y-stride and z-stride. That's what I mean by different semantics, and I think the difference is not enough to justify forking it. I'm ok either way, though. Eduardo >> >> Thanks for the feedback and the tip! >> >> Eduardo >> >>> On January 25, 2019 07:48:54 Eduardo Lima Mitev wrote: >>> This is an internal intrinsic intended to be injected by a (freedreno-specific) 'lower_sampler_io' pass that will be introduced later in this series; and consumed by ir3_compiler_nir. The intrinsic will load in SSA values for various constants for images (image_dims), namely the format's bytes-per-pixel, y-stride and z-stride; for which the backend compiler will emit the corresponding uniforms. const_index[0] is the image index, and const_index[1] is the index into image_dims' register file for a given image: 0 for bpp, 1 to y-stride and 2 for z-stride. --- src/compiler/nir/nir_intrinsics.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/compiler/nir/nir_intrinsics.py b/src/compiler/nir/nir_intrinsics.py index a5cc3f7401c..169c835d746 100644 --- a/src/compiler/nir/nir_intrinsics.py +++ b/src/compiler/nir/nir_intrinsics.py @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET], [CAN_ELIMINATE]) load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) # src[] = { offset }. const_index[] = { base, range } load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) +# src[] = { offset }. const_index[] = { image_idx, dim_idx } +load("image_stride", 1, [], [CAN_REORDER]) # Stores work the same way as loads, except now the first source is the value # to store and the second (and possibly third) source specify where to store -- 2.20.1 ___ mesa-dev mailing list mesa-...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> >>> >>> >>> >> ___ >> mesa-dev mailing list >> mesa-...@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
On 1/28/19 10:16 AM, Eduardo Lima Mitev wrote: > On 1/28/19 9:56 AM, Bas Nieuwenhuizen wrote: >> On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev wrote: >>> >>> On 1/26/19 5:34 PM, Jason Ekstrand wrote: Mind suffixing it with _ir3 or something since it's a back-end-specific intrinsic? Incidentally, this looks a lot like load_image_param_intel. >>> >>> Yes, I felt inclined to add the suffix but wasn't sure how good/bad a >>> practice it is, so I deferred it to review :). >>> >>> Now, I did a quick experiment and it turns out I can reuse >>> image_deref_load_param_intel as-is. The semantics are a bit different so >>> I would have to fork the comment to explain ir3 too. >>> >>> So, what about I remove the '_intel' suffix to that one and use if for >>> this ir3? >> >> Instructions that have different semantics for different drivers sound >> like a lot of potential for confusion and latent bugs to me. Is it >> really a problem to have both? >> > > It is not a problem at all, but I think it is preferable not to > duplicate this intrinsic if it serves the same purpose on both backends, > and only slightly differ in the specific set of params it holds: > > On both backends the intrinsic is used to represent registers at compile > time, that will be resolved to uniforms at shader run time. > > On intel, the params are offset, tiling, stride and swizzling; on ir3 > they are bytes-per-pixel, y-stride and z-stride. That's what I mean by > different semantics, and I think the difference is not enough to justify > forking it. > Actually, one can argue there is no semantic difference at all from NIR's point of view. The intrinsic just load certain image params on both backends, and it is only backend-specific what those params are. > I'm ok either way, though. > > Eduardo > >>> >>> Thanks for the feedback and the tip! >>> >>> Eduardo >>> On January 25, 2019 07:48:54 Eduardo Lima Mitev wrote: > This is an internal intrinsic intended to be injected by a > (freedreno-specific) 'lower_sampler_io' pass that will be introduced > later in this series; and consumed by ir3_compiler_nir. > > The intrinsic will load in SSA values for various constants > for images (image_dims), namely the format's bytes-per-pixel, > y-stride and z-stride; for which the backend compiler will emit > the corresponding uniforms. > > const_index[0] is the image index, and const_index[1] is the index > into image_dims' register file for a given image: 0 for bpp, 1 to > y-stride and 2 for z-stride. > --- > src/compiler/nir/nir_intrinsics.py | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/compiler/nir/nir_intrinsics.py > b/src/compiler/nir/nir_intrinsics.py > index a5cc3f7401c..169c835d746 100644 > --- a/src/compiler/nir/nir_intrinsics.py > +++ b/src/compiler/nir/nir_intrinsics.py > @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET], > [CAN_ELIMINATE]) > load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) > # src[] = { offset }. const_index[] = { base, range } > load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) > +# src[] = { offset }. const_index[] = { image_idx, dim_idx } > +load("image_stride", 1, [], [CAN_REORDER]) > > # Stores work the same way as loads, except now the first source is > the value > # to store and the second (and possibly third) source specify where to > store > -- > 2.20.1 > > ___ > mesa-dev mailing list > mesa-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> ___ >>> mesa-dev mailing list >>> mesa-...@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > ___ > mesa-dev mailing list > mesa-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Mesa-dev] [RFC 1/4] nir: Add a new intrinsic 'load_image_stride'
On Mon, Jan 28, 2019 at 9:38 AM Eduardo Lima Mitev wrote: > > On 1/26/19 5:34 PM, Jason Ekstrand wrote: > > Mind suffixing it with _ir3 or something since it's a back-end-specific > > intrinsic? Incidentally, this looks a lot like load_image_param_intel. > > > > Yes, I felt inclined to add the suffix but wasn't sure how good/bad a > practice it is, so I deferred it to review :). > > Now, I did a quick experiment and it turns out I can reuse > image_deref_load_param_intel as-is. The semantics are a bit different so > I would have to fork the comment to explain ir3 too. > > So, what about I remove the '_intel' suffix to that one and use if for > this ir3? Instructions that have different semantics for different drivers sound like a lot of potential for confusion and latent bugs to me. Is it really a problem to have both? > > Thanks for the feedback and the tip! > > Eduardo > > > On January 25, 2019 07:48:54 Eduardo Lima Mitev wrote: > > > >> This is an internal intrinsic intended to be injected by a > >> (freedreno-specific) 'lower_sampler_io' pass that will be introduced > >> later in this series; and consumed by ir3_compiler_nir. > >> > >> The intrinsic will load in SSA values for various constants > >> for images (image_dims), namely the format's bytes-per-pixel, > >> y-stride and z-stride; for which the backend compiler will emit > >> the corresponding uniforms. > >> > >> const_index[0] is the image index, and const_index[1] is the index > >> into image_dims' register file for a given image: 0 for bpp, 1 to > >> y-stride and 2 for z-stride. > >> --- > >> src/compiler/nir/nir_intrinsics.py | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/src/compiler/nir/nir_intrinsics.py > >> b/src/compiler/nir/nir_intrinsics.py > >> index a5cc3f7401c..169c835d746 100644 > >> --- a/src/compiler/nir/nir_intrinsics.py > >> +++ b/src/compiler/nir/nir_intrinsics.py > >> @@ -590,6 +590,8 @@ load("shared", 1, [BASE, ALIGN_MUL, ALIGN_OFFSET], > >> [CAN_ELIMINATE]) > >> load("push_constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) > >> # src[] = { offset }. const_index[] = { base, range } > >> load("constant", 1, [BASE, RANGE], [CAN_ELIMINATE, CAN_REORDER]) > >> +# src[] = { offset }. const_index[] = { image_idx, dim_idx } > >> +load("image_stride", 1, [], [CAN_REORDER]) > >> > >> # Stores work the same way as loads, except now the first source is > >> the value > >> # to store and the second (and possibly third) source specify where to > >> store > >> -- > >> 2.20.1 > >> > >> ___ > >> mesa-dev mailing list > >> mesa-...@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > > > > ___ > mesa-dev mailing list > mesa-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC 0/4] freedreno: Move some compiler offset computations to NIR
On Mon, Jan 28, 2019 at 3:32 AM Eduardo Lima Mitev wrote: > > On 1/26/19 12:42 AM, Rob Clark wrote: > > On Fri, Jan 25, 2019 at 10:48 AM Eduardo Lima Mitev > > wrote: > >> > >> There are a bunch of instructions emitted on ir3_compiler_nir related to > >> offset computations for IO opcodes (ssbo, image, etc). This small series > >> explores the possibility of moving these instructions to NIR, where we > >> have higher chances of optimizing them. > >> > >> The series introduces a new, freedreno specific NIR pass, > >> 'ir3_nir_lower_sampler_io' (final name not set). The pass is executed > >> early on ir3_optimize_nir(), and the goal is to centralize all these > >> computations there, hoping that later NIR passes will produce better > >> code than what is currently emitted. > > > > I can think of a few other things that would be interesting to lower > > to driver specific nir opcodes (imul and various lowering for tex > > instructions come to mind.. but probably also ubo and ssbo address > > calculation.. maybe it could even make sense for some of the single > > src alu instructions that translate into multiple ir3 instructions, > > not sure).. > > > > Yes, the plan is to abstract to NIR whatever brings us a benefit in > instruction stats. There is also the question of just simplifying the > backend compiler, provided we don't hurt produced code. yeah, simplifying nir->ir3 would itself be a worthwhile thing > > Are you thinking about having separate passes for each? I guess at > > least for alu instructions we might able to use nir_algebraic so > > having things split up might be easier. > > > > I haven't thought too much about this yet, but it seems to make sense > having at least 2 passes, one for I/O and one for ALUs. I haven't thought about it too much, but I think this makes sense > >> So far, we have just implemented byte-offset computation for image store > >> and atomics. This seemed like a good first target given the amount of > >> instructions being emitted for it by the backend. > >> > >> This is an RFC series because there are a few open questions, but we > >> wanted to gather feedback already now, in case this effort is something > >> not worth it; and also hoping that somebody else will give it a try > >> against other shaders and on other gens (we have just tried this on > >> a5xx). > >> > >> * We have so far been unable to see any improvement in generated code > >> (not a penalty either). shader-db has not been specially useful. Few > >> shaders there exercise image store or image atomic ops, and of those > >> that do, most require higher versions of GLSL than what freedreno > >> supports, so they get skipped. The few that do actually run, don't > >> show any meaningful difference. > > > > I guess it would be easy enough to construct shaders that would > > benefit from this, but maybe that is cheating.. > > > > Possibly UBO and SSBO is a better target, I guess there you might be > > more likely to see patterns of access of successive elements (ie. > > foo[idx], foo[idx+1], etc)? > > > > I took a first stab at SSBO's load/store/atomic, where the offset is > divided by 4 in the backend, but was bitten by IR3_STGB requiring both > the original byte-offset and the dword-offset (in src1 and src2 > respectively). So trivially emitting a nir_shr on the offset didn't buy > us anything. I have in the backlog to revisit this, turning the offset > into a 2-component reg so we can hold the original byte-offset and the > offset divided by 4. hmm, or just two src's? Anyways, it seems like there is some potential to fold that shift into other math, perhaps if you had a shader that did something like 'somessbo[val << 1]'? fwiw, I think ssbo's and images get a bit simpler, or rather less alu instructions.. a5xx and earlier wanted both idx/coords and offset (Ie. idx/coords used for clamping out of bounds access, but offset is what is actually used to fetch/store). With a6xx it becomes just idx/coords, no offset calc.. which *hopefully* means we can deal with tiled images more easily. But a6xx has it's own challenge with one of the instruction srcs being a vecN with both input and output values. See 'instr_cat6_a6xx_t'. (note: we don't use ldc for UBOs yet, still sticking with the general purpose ldg. Maybe we should switch to get rid of some address calculation.) > > Anyways, since we don't try to do (and I'd rather not do) any sort of > > CSE post nir->ir3 I think starting to introduce more ir3 specific > > nir->nir lowering seems like a thing we need, so I'm pretty happy that > > someone is looking at this :-) > > > > Thanks, that's encouraging. Lets see how far we can get :). > cool BR, -R ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH AUTOSEL 4.20 161/304] drm/msm/dsi: fix dsi clock names in DSI 10nm PLL driver
From: Abhinav Kumar [ Upstream commit c1866d44d149a1ea5c303632114fb6aa08cfd263 ] Fix the dsi clock names in the DSI 10nm PLL driver to match the names in the dispcc driver as those are according to the clock plan of the chipset. Changes in v2: - Update the clock diagram with the new clock name Reviewed-by: Sean Paul Signed-off-by: Abhinav Kumar Signed-off-by: Sean Paul Signed-off-by: Rob Clark Signed-off-by: Sasha Levin --- drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c index 41bec570c518..31205625c734 100644 --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c @@ -17,7 +17,7 @@ * || * || * +-+ | +--+ | ++ - * dsi0vco_clk ---| out_div |--o--| divl_3_0 |--o--| /8 |-- dsi0pllbyte + * dsi0vco_clk ---| out_div |--o--| divl_3_0 |--o--| /8 |-- dsi0_phy_pll_out_byteclk * +-+ | +--+ | ++ * || * || dsi0_pll_by_2_bit_clk @@ -25,7 +25,7 @@ * || ++ | |\ dsi0_pclk_mux * ||--| /2 |--o--| \ | * || ++ | \ | +-+ - * |--| |--o--| div_7_4 |-- dsi0pll + * |--| |--o--| div_7_4 |-- dsi0_phy_pll_out_dsiclk * |--| / +-+ * | +-+ | / * ---| /4? |--o--|/ @@ -690,7 +690,7 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) hws[num++] = hw; - snprintf(clk_name, 32, "dsi%dpllbyte", pll_10nm->id); + snprintf(clk_name, 32, "dsi%d_phy_pll_out_byteclk", pll_10nm->id); snprintf(parent, 32, "dsi%d_pll_bit_clk", pll_10nm->id); /* DSI Byte clock = VCO_CLK / OUT_DIV / BIT_DIV / 8 */ @@ -739,7 +739,7 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) hws[num++] = hw; - snprintf(clk_name, 32, "dsi%dpll", pll_10nm->id); + snprintf(clk_name, 32, "dsi%d_phy_pll_out_dsiclk", pll_10nm->id); snprintf(parent, 32, "dsi%d_pclk_mux", pll_10nm->id); /* PIX CLK DIV : DIV_CTRL_7_4*/ -- 2.19.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH AUTOSEL 4.20 162/304] drm/msm: dpu: Only check flush register against pending flushes
From: Sean Paul [ Upstream commit 5f79e03b1f7c1b2cf0019ce6365fe5d52629813d ] There exists a case where a flush of a plane/dma may have been triggered & started from an async commit. If that plane/dma is subsequently disabled by the next commit, the flush register will continue to hold the flush bit for the disabled plane. Since the bit remains active, pending_kickoff_cnt will never decrement and we'll miss frame_done events. This patch limits the check of flush_register to include only those bits which have been updated with the latest commit. Changes in v2: - None Reviewed-by: Jeykumar Sankaran Signed-off-by: Sean Paul Signed-off-by: Rob Clark Signed-off-by: Sasha Levin --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index 84de385a9f62..60f146f02b77 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx) if (hw_ctl && hw_ctl->ops.get_flush_register) flush_register = hw_ctl->ops.get_flush_register(hw_ctl); - if (flush_register == 0) + if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl))) new_cnt = atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0); spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags); -- 2.19.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH AUTOSEL 4.19 137/258] drm/msm/dsi: fix dsi clock names in DSI 10nm PLL driver
From: Abhinav Kumar [ Upstream commit c1866d44d149a1ea5c303632114fb6aa08cfd263 ] Fix the dsi clock names in the DSI 10nm PLL driver to match the names in the dispcc driver as those are according to the clock plan of the chipset. Changes in v2: - Update the clock diagram with the new clock name Reviewed-by: Sean Paul Signed-off-by: Abhinav Kumar Signed-off-by: Sean Paul Signed-off-by: Rob Clark Signed-off-by: Sasha Levin --- drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c index 41bec570c518..31205625c734 100644 --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c @@ -17,7 +17,7 @@ * || * || * +-+ | +--+ | ++ - * dsi0vco_clk ---| out_div |--o--| divl_3_0 |--o--| /8 |-- dsi0pllbyte + * dsi0vco_clk ---| out_div |--o--| divl_3_0 |--o--| /8 |-- dsi0_phy_pll_out_byteclk * +-+ | +--+ | ++ * || * || dsi0_pll_by_2_bit_clk @@ -25,7 +25,7 @@ * || ++ | |\ dsi0_pclk_mux * ||--| /2 |--o--| \ | * || ++ | \ | +-+ - * |--| |--o--| div_7_4 |-- dsi0pll + * |--| |--o--| div_7_4 |-- dsi0_phy_pll_out_dsiclk * |--| / +-+ * | +-+ | / * ---| /4? |--o--|/ @@ -690,7 +690,7 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) hws[num++] = hw; - snprintf(clk_name, 32, "dsi%dpllbyte", pll_10nm->id); + snprintf(clk_name, 32, "dsi%d_phy_pll_out_byteclk", pll_10nm->id); snprintf(parent, 32, "dsi%d_pll_bit_clk", pll_10nm->id); /* DSI Byte clock = VCO_CLK / OUT_DIV / BIT_DIV / 8 */ @@ -739,7 +739,7 @@ static int pll_10nm_register(struct dsi_pll_10nm *pll_10nm) hws[num++] = hw; - snprintf(clk_name, 32, "dsi%dpll", pll_10nm->id); + snprintf(clk_name, 32, "dsi%d_phy_pll_out_dsiclk", pll_10nm->id); snprintf(parent, 32, "dsi%d_pclk_mux", pll_10nm->id); /* PIX CLK DIV : DIV_CTRL_7_4*/ -- 2.19.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH AUTOSEL 4.19 138/258] drm/msm: dpu: Only check flush register against pending flushes
From: Sean Paul [ Upstream commit 5f79e03b1f7c1b2cf0019ce6365fe5d52629813d ] There exists a case where a flush of a plane/dma may have been triggered & started from an async commit. If that plane/dma is subsequently disabled by the next commit, the flush register will continue to hold the flush bit for the disabled plane. Since the bit remains active, pending_kickoff_cnt will never decrement and we'll miss frame_done events. This patch limits the check of flush_register to include only those bits which have been updated with the latest commit. Changes in v2: - None Reviewed-by: Jeykumar Sankaran Signed-off-by: Sean Paul Signed-off-by: Rob Clark Signed-off-by: Sasha Levin --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index 14fc7c2a6bb7..c9962a36b86b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx) if (hw_ctl && hw_ctl->ops.get_flush_register) flush_register = hw_ctl->ops.get_flush_register(hw_ctl); - if (flush_register == 0) + if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl))) new_cnt = atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0); spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags); -- 2.19.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v2] drm/msm/dpu: remove struct encoder_kickoff_params
From: Bruce Wang The contents of struct encoder_kickoff_params are never used. Remove the structure and all remnants of it from function calls. Changes in v2 (seanpaul): - Actually remove the struct (Jeykumar) Cc: Jeykumar Sankaran Signed-off-by: Bruce Wang Signed-off-by: Sean Paul --- Sending v2 since Bruce has returned to school. Sean drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 7 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 13 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h| 3 +-- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c| 5 ++--- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c| 5 ++--- 6 files changed, 11 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 4e4b64821c9e8..f3a37aa4d0988 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -716,11 +716,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async) * may delay and flush at an irq event (e.g. ppdone) */ drm_for_each_encoder_mask(encoder, crtc->dev, - crtc->state->encoder_mask) { - struct dpu_encoder_kickoff_params params = { 0 }; - dpu_encoder_prepare_for_kickoff(encoder, ¶ms, async); - } - + crtc->state->encoder_mask) + dpu_encoder_prepare_for_kickoff(encoder, async); if (!async) { /* wait for frame_event_done completion */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 941ac25d2a023..b2c86762bcf29 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1747,15 +1747,14 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work) nsecs_to_jiffies(ktime_to_ns(wakeup_time))); } -void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc, - struct dpu_encoder_kickoff_params *params, bool async) +void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc, bool async) { struct dpu_encoder_virt *dpu_enc; struct dpu_encoder_phys *phys; bool needs_hw_reset = false; unsigned int i; - if (!drm_enc || !params) { + if (!drm_enc) { DPU_ERROR("invalid args\n"); return; } @@ -1769,7 +1768,7 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc, phys = dpu_enc->phys_encs[i]; if (phys) { if (phys->ops.prepare_for_kickoff) - phys->ops.prepare_for_kickoff(phys, params); + phys->ops.prepare_for_kickoff(phys); if (phys->enable_state == DPU_ENC_ERR_NEEDS_HW_RESET) needs_hw_reset = true; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 3f5dafe00580e..d77f74fb26d4b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -37,15 +37,6 @@ struct dpu_encoder_hw_resources { enum dpu_intf_mode intfs[INTF_MAX]; }; -/** - * dpu_encoder_kickoff_params - info encoder requires at kickoff - * @affected_displays: bitmask, bit set means the ROI of the commit lies within - * the bounds of the physical display at the bit index - */ -struct dpu_encoder_kickoff_params { - unsigned long affected_displays; -}; - /** * dpu_encoder_get_hw_resources - Populate table of required hardware resources * @encoder: encoder pointer @@ -88,11 +79,9 @@ void dpu_encoder_register_frame_event_callback(struct drm_encoder *encoder, * Immediately: if no previous commit is outstanding. * Delayed: Block until next trigger can be issued. * @encoder: encoder pointer - * @params:kickoff time parameters * @async: true if this is an asynchronous commit */ -void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder, - struct dpu_encoder_kickoff_params *params, bool async); +void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder, bool async); /** * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from previous diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h index 44e6f8b68e70d..db94f3d3bea31 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -144,8 +144,7 @@ struct dpu_encoder_phys_ops { int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc); int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
[Freedreno] [PATCH 4/4] drm/msm: dpu: Don't queue the frame_done watchdog for cursor
From: Sean Paul In the case of an async/cursor update, we don't wait for the frame_done event, which means handle_frame_done is never called, and the frame_done watchdog isn't canceled. Currently, this results in a frame_done timeout every time the cursor moves without a synchronous frame following it up before the timeout expires. Since we don't wait for frame_done, and don't handle it, we shouldn't modify the watchdog. Signed-off-by: Sean Paul --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 +-- 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 51e46b206c73e..05145cf9fb408 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1794,7 +1794,6 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) { struct dpu_encoder_virt *dpu_enc; struct dpu_encoder_phys *phys; - unsigned long timeout_ms; ktime_t wakeup_time; unsigned int i; @@ -1807,12 +1806,20 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) trace_dpu_enc_kickoff(DRMID(drm_enc)); - timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 / - drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode); + /* +* Asynchronous frames don't handle FRAME_DONE events. As such, they +* shouldn't enable the frame_done watchdog since it will always time +* out. +*/ + if (!async) { + unsigned long timeout_ms; + timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 / + drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode); - atomic_set(&dpu_enc->frame_done_timeout_ms, timeout_ms); - mod_timer(&dpu_enc->frame_done_timer, - jiffies + msecs_to_jiffies(timeout_ms)); + atomic_set(&dpu_enc->frame_done_timeout_ms, timeout_ms); + mod_timer(&dpu_enc->frame_done_timer, + jiffies + msecs_to_jiffies(timeout_ms)); + } /* All phys encs are ready to go, trigger the kickoff */ _dpu_encoder_kickoff_phys(dpu_enc, async); -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh
From: Sean Paul Use the drm_mode_vrefresh helper where we need refresh rate in case vrefresh is empty. Signed-off-by: Sean Paul --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c| 2 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 941ac25d2a023..dd71cb6ba4f5c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -522,8 +522,8 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector, list_for_each_entry(cur_mode, &connector->modes, head) { if (cur_mode->vdisplay == adj_mode->vdisplay && - cur_mode->hdisplay == adj_mode->hdisplay && - cur_mode->vrefresh == adj_mode->vrefresh) { + cur_mode->hdisplay == adj_mode->hdisplay && + drm_mode_vrefresh(cur_mode) == drm_mode_vrefresh(adj_mode)) { adj_mode->private = cur_mode->private; adj_mode->private_flags |= cur_mode->private_flags; } @@ -1805,7 +1805,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) atomic_set(&dpu_enc->frame_done_timeout, DPU_FRAME_DONE_TIMEOUT * 1000 / - drm_enc->crtc->state->adjusted_mode.vrefresh); + drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode)); mod_timer(&dpu_enc->frame_done_timer, jiffies + ((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000)); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 99ab5ca9bed3b..f21163313d635 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -404,7 +404,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config( return; } - tc_cfg.vsync_count = vsync_hz / (mode->vtotal * mode->vrefresh); + tc_cfg.vsync_count = vsync_hz / + (mode->vtotal * drm_mode_vrefresh(mode)); /* enable external TE after kickoff to avoid premature autorefresh */ tc_cfg.hw_vsync_mode = 0; @@ -424,7 +425,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config( DPU_DEBUG_CMDENC(cmd_enc, "tc %d vsync_clk_speed_hz %u vtotal %u vrefresh %u\n", phys_enc->hw_pp->idx - PINGPONG_0, vsync_hz, - mode->vtotal, mode->vrefresh); + mode->vtotal, drm_mode_vrefresh(mode)); DPU_DEBUG_CMDENC(cmd_enc, "tc %d enable %u start_pos %u rd_ptr_irq %u\n", phys_enc->hw_pp->idx - PINGPONG_0, tc_enable, tc_cfg.start_pos, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index b01183b309b9e..da1f727d74957 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -387,7 +387,7 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane, ot_params.width = drm_rect_width(&pdpu->pipe_cfg.src_rect); ot_params.height = drm_rect_height(&pdpu->pipe_cfg.src_rect); ot_params.is_wfd = !pdpu->is_rt_pipe; - ot_params.frame_rate = crtc->mode.vrefresh; + ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode); ot_params.vbif_idx = VBIF_RT; ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl; ot_params.rd = true; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c index c1962f29ec7d6..6341ac010f7de 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c @@ -59,10 +59,10 @@ static int pingpong_tearcheck_setup(struct drm_encoder *encoder, return -EINVAL; } - total_lines_x100 = mode->vtotal * mode->vrefresh; + total_lines_x100 = mode->vtotal * drm_mode_vrefresh(mode); if (!total_lines_x100) { DRM_DEV_ERROR(dev, "%s: vtotal(%d) or vrefresh(%d) is 0\n", - __func__, mode->vtotal, mode->vrefresh); + __func__, mode->vtotal, drm_mode_vrefresh(mode)); return -EINVAL; } -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 2/4] drm/msm: dpu: Simplify frame_done watchdog timeout calculation
From: Sean Paul Instead of setting the timeout and then immediately reading it back (along with the hand-rolled msecs_to_jiffies calculation), just calculate it once and set it in both places at the same time. Signed-off-by: Sean Paul --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index dd71cb6ba4f5c..83a4c47dbed2d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1791,6 +1791,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) { struct dpu_encoder_virt *dpu_enc; struct dpu_encoder_phys *phys; + unsigned long timeout_ms; ktime_t wakeup_time; unsigned int i; @@ -1803,11 +1804,12 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) trace_dpu_enc_kickoff(DRMID(drm_enc)); - atomic_set(&dpu_enc->frame_done_timeout, - DPU_FRAME_DONE_TIMEOUT * 1000 / - drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode)); - mod_timer(&dpu_enc->frame_done_timer, jiffies + - ((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000)); + timeout_ms = DPU_FRAME_DONE_TIMEOUT * 1000 / + drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode); + + atomic_set(&dpu_enc->frame_done_timeout, timeout_ms); + mod_timer(&dpu_enc->frame_done_timer, + jiffies + msecs_to_jiffies(timeout_ms)); /* All phys encs are ready to go, trigger the kickoff */ _dpu_encoder_kickoff_phys(dpu_enc, async); -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 3/4] drm/msm: dpu: Untangle frame_done timeout units
From: Sean Paul There exists a bunch of confusion as to what the actual units of frame_done is: - The definition states it's in # of frames - CRTC treats it like it's ms - frame_done_timeout comment thinks it's Hz, but it stores ms - frame_done timer is setup such that it _should_ be in frames, but the timeout is super long So this patch tries to interpret what the driver really wants. I've de-centralized the #define since the consumers are expecting different units. For crtc, we just use 60ms since that's what it was doing before. Perhaps we could get fancy and scale with vrefresh, but that's for another time. For encoder, fix the comments and rename frame_done_timeout so it's obvious what the units are. In practice, frame_done_timeout is really just checked against 0 || !0, which I guess is why the units being wrong didn't matter. I've also dropped the timeout from the previous 60 frames to 5. That seems like more than enough time to give up on a frame, and my guess is that no one intended for the timeout to _actually_ be 60 frames. Signed-off-by: Sean Paul --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 5 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 3 --- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 4e4b64821c9e8..b0b394af2a7ef 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -46,6 +46,9 @@ #define LEFT_MIXER 0 #define RIGHT_MIXER 1 +/* timeout in ms waiting for frame done */ +#define DPU_CRTC_FRAME_DONE_TIMEOUT_MS 60 + static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) { struct msm_drm_private *priv = crtc->dev->dev_private; @@ -683,7 +686,7 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc) DPU_ATRACE_BEGIN("frame done completion wait"); ret = wait_for_completion_timeout(&dpu_crtc->frame_done_comp, - msecs_to_jiffies(DPU_FRAME_DONE_TIMEOUT)); + msecs_to_jiffies(DPU_CRTC_FRAME_DONE_TIMEOUT_MS)); if (!ret) { DRM_ERROR("frame done wait timed out, ret:%d\n", ret); rc = -ETIMEDOUT; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 83a4c47dbed2d..51e46b206c73e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -69,6 +69,9 @@ #define MAX_VDISPLAY_SPLIT 1080 +/* timeout in frames waiting for frame done */ +#define DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES 5 + /** * enum dpu_enc_rc_events - events for resource control state machine * @DPU_ENC_RC_EVENT_KICKOFF: @@ -158,7 +161,7 @@ enum dpu_enc_rc_states { * Bit0 = phys_encs[0] etc. * @crtc_frame_event_cb: callback handler for frame event * @crtc_frame_event_cb_data: callback handler private data - * @frame_done_timeout:frame done timeout in Hz + * @frame_done_timeout_ms: frame done timeout in ms * @frame_done_timer: watchdog timer for frame done event * @vsync_event_timer: vsync timer * @disp_info: local copy of msm_display_info struct @@ -196,7 +199,7 @@ struct dpu_encoder_virt { void (*crtc_frame_event_cb)(void *, u32 event); void *crtc_frame_event_cb_data; - atomic_t frame_done_timeout; + atomic_t frame_done_timeout_ms; struct timer_list frame_done_timer; struct timer_list vsync_event_timer; @@ -1184,7 +1187,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) } /* after phys waits for frame-done, should be no more frames pending */ - if (atomic_xchg(&dpu_enc->frame_done_timeout, 0)) { + if (atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) { DPU_ERROR("enc%d timeout pending\n", drm_enc->base.id); del_timer_sync(&dpu_enc->frame_done_timer); } @@ -1341,7 +1344,7 @@ static void dpu_encoder_frame_done_callback( } if (!dpu_enc->frame_busy_mask[0]) { - atomic_set(&dpu_enc->frame_done_timeout, 0); + atomic_set(&dpu_enc->frame_done_timeout_ms, 0); del_timer(&dpu_enc->frame_done_timer); dpu_encoder_resource_control(drm_enc, @@ -1804,10 +1807,10 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) trace_dpu_enc_kickoff(DRMID(drm_enc)); - timeout_ms = DPU_FRAME_DONE_TIMEOUT * 1000 / + timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 / drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode); - atomic_set(&dpu_enc->frame_done_timeout, timeout_ms); + atomic_set(&dpu_enc->frame_done_timeout_ms, timeout_ms);
Re: [Freedreno] [PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh
On 2019-01-28 12:42, Sean Paul wrote: From: Sean Paul Use the drm_mode_vrefresh helper where we need refresh rate in case vrefresh is empty. Signed-off-by: Sean Paul Reviewed-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 6 +++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c| 2 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 941ac25d2a023..dd71cb6ba4f5c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -522,8 +522,8 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector, list_for_each_entry(cur_mode, &connector->modes, head) { if (cur_mode->vdisplay == adj_mode->vdisplay && - cur_mode->hdisplay == adj_mode->hdisplay && - cur_mode->vrefresh == adj_mode->vrefresh) { + cur_mode->hdisplay == adj_mode->hdisplay && + drm_mode_vrefresh(cur_mode) == drm_mode_vrefresh(adj_mode)) { adj_mode->private = cur_mode->private; adj_mode->private_flags |= cur_mode->private_flags; } @@ -1805,7 +1805,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) atomic_set(&dpu_enc->frame_done_timeout, DPU_FRAME_DONE_TIMEOUT * 1000 / - drm_enc->crtc->state->adjusted_mode.vrefresh); + drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode)); mod_timer(&dpu_enc->frame_done_timer, jiffies + ((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000)); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 99ab5ca9bed3b..f21163313d635 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -404,7 +404,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config( return; } - tc_cfg.vsync_count = vsync_hz / (mode->vtotal * mode->vrefresh); + tc_cfg.vsync_count = vsync_hz / + (mode->vtotal * drm_mode_vrefresh(mode)); /* enable external TE after kickoff to avoid premature autorefresh */ tc_cfg.hw_vsync_mode = 0; @@ -424,7 +425,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config( DPU_DEBUG_CMDENC(cmd_enc, "tc %d vsync_clk_speed_hz %u vtotal %u vrefresh %u\n", phys_enc->hw_pp->idx - PINGPONG_0, vsync_hz, - mode->vtotal, mode->vrefresh); + mode->vtotal, drm_mode_vrefresh(mode)); DPU_DEBUG_CMDENC(cmd_enc, "tc %d enable %u start_pos %u rd_ptr_irq %u\n", phys_enc->hw_pp->idx - PINGPONG_0, tc_enable, tc_cfg.start_pos, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index b01183b309b9e..da1f727d74957 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -387,7 +387,7 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane, ot_params.width = drm_rect_width(&pdpu->pipe_cfg.src_rect); ot_params.height = drm_rect_height(&pdpu->pipe_cfg.src_rect); ot_params.is_wfd = !pdpu->is_rt_pipe; - ot_params.frame_rate = crtc->mode.vrefresh; + ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode); ot_params.vbif_idx = VBIF_RT; ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl; ot_params.rd = true; diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c index c1962f29ec7d6..6341ac010f7de 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c @@ -59,10 +59,10 @@ static int pingpong_tearcheck_setup(struct drm_encoder *encoder, return -EINVAL; } - total_lines_x100 = mode->vtotal * mode->vrefresh; + total_lines_x100 = mode->vtotal * drm_mode_vrefresh(mode); if (!total_lines_x100) { DRM_DEV_ERROR(dev, "%s: vtotal(%d) or vrefresh(%d) is 0\n", - __func__, mode->vtotal, mode->vrefresh); + __func__, mode->vtotal, drm_mode_vrefresh(mode)); return -EINVAL; } ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/4] drm/msm: dpu: Simplify frame_done watchdog timeout calculation
On 2019-01-28 12:42, Sean Paul wrote: From: Sean Paul Instead of setting the timeout and then immediately reading it back (along with the hand-rolled msecs_to_jiffies calculation), just calculate it once and set it in both places at the same time. Signed-off-by: Sean Paul Reviewed-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index dd71cb6ba4f5c..83a4c47dbed2d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1791,6 +1791,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) { struct dpu_encoder_virt *dpu_enc; struct dpu_encoder_phys *phys; + unsigned long timeout_ms; ktime_t wakeup_time; unsigned int i; @@ -1803,11 +1804,12 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) trace_dpu_enc_kickoff(DRMID(drm_enc)); - atomic_set(&dpu_enc->frame_done_timeout, - DPU_FRAME_DONE_TIMEOUT * 1000 / - drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode)); - mod_timer(&dpu_enc->frame_done_timer, jiffies + - ((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000)); + timeout_ms = DPU_FRAME_DONE_TIMEOUT * 1000 / + drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode); + + atomic_set(&dpu_enc->frame_done_timeout, timeout_ms); + mod_timer(&dpu_enc->frame_done_timer, + jiffies + msecs_to_jiffies(timeout_ms)); /* All phys encs are ready to go, trigger the kickoff */ _dpu_encoder_kickoff_phys(dpu_enc, async); ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v2] drm: add definitions for DP Audio/Video compliance tests
This change adds definitions needed for DP audio compliance testing. It also adds missing definition for DP video compliance. Changes in V2: -- Delete cover letter for this patch. -- Move the description from cover letter into patch commit message. -- Remove DPU from subject prefix Signed-off-by: Chandan Uddaraju --- include/drm/drm_dp_helper.h | 13 + 1 file changed, 13 insertions(+) diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 5736c94..e688e05 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -556,6 +556,8 @@ # define DP_TEST_LINK_EDID_READ(1 << 2) # define DP_TEST_LINK_PHY_TEST_PATTERN (1 << 3) /* DPCD >= 1.1 */ # define DP_TEST_LINK_FAUX_PATTERN (1 << 4) /* DPCD >= 1.2 */ +# define DP_TEST_LINK_AUDIO_PATTERN (1 << 5) /* DPCD >= 1.2 */ +# define DP_TEST_LINK_AUDIO_DISABLED_VIDEO (1 << 6) /* DPCD >= 1.2 */ #define DP_TEST_LINK_RATE 0x219 # define DP_LINK_RATE_162 (0x6) @@ -604,6 +606,7 @@ # define DP_COLOR_FORMAT_RGB(0 << 1) # define DP_COLOR_FORMAT_YCbCr422 (1 << 1) # define DP_COLOR_FORMAT_YCbCr444 (2 << 1) +# define DP_TEST_DYNAMIC_RANGE_VESA (0 << 3) # define DP_TEST_DYNAMIC_RANGE_CEA (1 << 3) # define DP_TEST_YCBCR_COEFFICIENTS (1 << 4) # define DP_YCBCR_COEFFICIENTS_ITU601 (0 << 4) @@ -653,6 +656,16 @@ #define DP_TEST_SINK 0x270 # define DP_TEST_SINK_START(1 << 0) +#define DP_TEST_AUDIO_MODE 0x271 +#define DP_TEST_AUDIO_PATTERN_TYPE 0x272 +#define DP_TEST_AUDIO_PERIOD_CH1 0x273 +#define DP_TEST_AUDIO_PERIOD_CH2 0x274 +#define DP_TEST_AUDIO_PERIOD_CH3 0x275 +#define DP_TEST_AUDIO_PERIOD_CH4 0x276 +#define DP_TEST_AUDIO_PERIOD_CH5 0x277 +#define DP_TEST_AUDIO_PERIOD_CH6 0x278 +#define DP_TEST_AUDIO_PERIOD_CH7 0x279 +#define DP_TEST_AUDIO_PERIOD_CH8 0x27A #define DP_FEC_STATUS 0x280/* 1.4 */ # define DP_FEC_DECODE_EN_DETECTED (1 << 0) -- 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] [DPU PATCH] drm: add definitions for DP Audio/Video compliance tests
On 2019-01-22 11:05, Sean Paul wrote: On Tue, Jan 01, 2019 at 11:15:25PM -0800, Chandan Uddaraju wrote: This seems fine to me. Could you please: - delete the cover letter - move the cover letter description into the patch commit msg - strip DPU from the subject prefix - send it to dri-devel, maintaining the cc's you have here With that, Reviewed-by: Sean Paul Sean Hello Sean, Addressed your comments and uploaded new patch. thanks Chandan Signed-off-by: Chandan Uddaraju --- include/drm/drm_dp_helper.h | 13 + 1 file changed, 13 insertions(+) diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 5736c94..e688e05 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -556,6 +556,8 @@ # define DP_TEST_LINK_EDID_READ(1 << 2) # define DP_TEST_LINK_PHY_TEST_PATTERN (1 << 3) /* DPCD >= 1.1 */ # define DP_TEST_LINK_FAUX_PATTERN (1 << 4) /* DPCD >= 1.2 */ +# define DP_TEST_LINK_AUDIO_PATTERN (1 << 5) /* DPCD >= 1.2 */ +# define DP_TEST_LINK_AUDIO_DISABLED_VIDEO (1 << 6) /* DPCD >= 1.2 */ #define DP_TEST_LINK_RATE 0x219 # define DP_LINK_RATE_162 (0x6) @@ -604,6 +606,7 @@ # define DP_COLOR_FORMAT_RGB(0 << 1) # define DP_COLOR_FORMAT_YCbCr422 (1 << 1) # define DP_COLOR_FORMAT_YCbCr444 (2 << 1) +# define DP_TEST_DYNAMIC_RANGE_VESA (0 << 3) # define DP_TEST_DYNAMIC_RANGE_CEA (1 << 3) # define DP_TEST_YCBCR_COEFFICIENTS (1 << 4) # define DP_YCBCR_COEFFICIENTS_ITU601 (0 << 4) @@ -653,6 +656,16 @@ #define DP_TEST_SINK 0x270 # define DP_TEST_SINK_START(1 << 0) +#define DP_TEST_AUDIO_MODE 0x271 +#define DP_TEST_AUDIO_PATTERN_TYPE 0x272 +#define DP_TEST_AUDIO_PERIOD_CH1 0x273 +#define DP_TEST_AUDIO_PERIOD_CH2 0x274 +#define DP_TEST_AUDIO_PERIOD_CH3 0x275 +#define DP_TEST_AUDIO_PERIOD_CH4 0x276 +#define DP_TEST_AUDIO_PERIOD_CH5 0x277 +#define DP_TEST_AUDIO_PERIOD_CH6 0x278 +#define DP_TEST_AUDIO_PERIOD_CH7 0x279 +#define DP_TEST_AUDIO_PERIOD_CH8 0x27A #define DP_FEC_STATUS 0x280/* 1.4 */ # define DP_FEC_DECODE_EN_DETECTED (1 << 0) -- 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 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v5 0/8] drm/msm/dsi: Get PHY ref clocks from the DT
Hi, this series has gone through multiple rounds of review and there are no outstanding comments. It seems it should be ready to land, or is there anything left that needs to be addressed? Thanks Matthias On Wed, Dec 19, 2018 at 03:55:20PM -0800, Matthias Kaehlcke wrote: > The MSM DSI PHY drivers currently hardcode the name and the rate of > the PHY ref clock. Get the ref clock from the device tree instead. > > Note: testing of this series was limited to SDM845 and the 10nm PHY > > Major changes in v5: > - none (see per-patch change log for minor changes) > > Major changes in v4: > - always use parent rate for 28nm and 28nm 8960 PHYs > > Major changes in v3: > - keep supporting DTs without ref clock for the 28nm and the 28nm > 8960 PHYs > - added patch to add ref clock to qcom-apq8064.dtsi > > Major changes in v2: > - apply to all MSM DSI PHY drivers, not only 10nm > > Matthias Kaehlcke (8): > dt-bindings: msm/dsi: Add ref clock for PHYs > drm/msm/dsi: 28nm 8960 PHY: Get ref clock from the DT > drm/msm/dsi: 28nm PHY: Get ref clock from the DT > drm/msm/dsi: 14nm PHY: Get ref clock from the DT > drm/msm/dsi: 10nm PHY: Get ref clock from the DT > arm64: dts: qcom: msm8916: Set 'xo_board' as ref clock of the DSI PHY > arm64: dts: sdm845: Set 'bi_tcxo' as ref clock of the DSI PHYs > ARM: dts: qcom-apq8064: Set 'xo_board' as ref clock of the DSI PHY > > .../devicetree/bindings/display/msm/dsi.txt | 1 + > arch/arm/boot/dts/qcom-apq8064.dtsi | 5 +-- > arch/arm64/boot/dts/qcom/msm8916.dtsi | 5 +-- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 +++--- > drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c| 20 +-- > drivers/gpu/drm/msm/dsi/pll/dsi_pll_14nm.c| 23 +--- > drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm.c| 36 +-- > .../gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c | 24 ++--- > 8 files changed, 92 insertions(+), 32 deletions(-) > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno