Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi Stephen On 12/14/2022 4:29 PM, Stephen Boyd wrote: Quoting Doug Anderson (2022-12-14 16:14:42) Hi, On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar wrote: Hi Doug On 12/14/2022 2:29 PM, Doug Anderson wrote: Hi, On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote: There are 3 possible interrupt sources are handled by DP controller, HPDstatus, Controller state changes and Aux read/write transaction. At every irq, DP controller have to check isr status of every interrupt sources and service the interrupt if its isr status bits shows interrupts are pending. There is potential race condition may happen at current aux isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() even irq is not for aux read or write transaction. This may cause aux read transaction return premature if host aux data read is in the middle of waiting for sink to complete transferring data to host while irq happen. This will cause host's receiving buffer contains unexpected data. This patch fixes this problem by checking aux isr and return immediately at aux isr handler if there are no any isr status bits set. Follows are the signature at kernel logs when problem happen, EDID has corrupt header panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index d030a93..8f8b12a 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) isr = dp_catalog_aux_get_irq(aux->catalog); + /* +* if this irq is not for aux transfer, +* then return immediately +*/ Why do you need 4 lines for a comment that fits on one line? Yes, we can fit this to one line. + if (!isr) + return; I can confirm that this works for me. I could reproduce the EDID problems in the past and I can't after this patch. ...so I could give a: Tested-by: Douglas Anderson I'm not an expert on this part of the code, so feel free to ignore my other comments if everyone else thinks this patch is fine as-is, but to me something here feels a little fragile. It feels a little weird that we'll "complete" for _any_ interrupt that comes through now rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() to specifically identify interrupts that caused the end of the transfer. I guess that idea is that every possible interrupt we get causes the end of the transfer? -Doug So this turned out to be more tricky and was a good finding from kuogee. In the bad EDID case, it was technically not bad EDID. What was happening was, the VIDEO_READY interrupt was continuously firing. Ideally, this should fire only once but due to some error condition it kept firing. We dont exactly know why yet what was the error condition making it continuously fire. This is a great detail that is missing from the commit text. Yup, we should update this. In the DP ISR, the dp_aux_isr() gets called even if it was not an aux interrupt which fired (so the call flow in this case was dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() So we should certainly have some protection to return early from this routine if there was no aux interrupt which fired. I'm not sure that's a race condition though, more like a problem where the completion is called unconditionally? hmm ... True. Which is what this fix is doing. Its not completing any interrupt, its just returning early if no aux interrupt fired. ...but the whole problem was that it was doing the complete() at the end, right? Kuogee even mentioned that in the commit message. Specifically, I checked dp_aux_native_handler() and dp_aux_i2c_handler(), both of which are passed the "isr". Unless I messed up, both functions already were no-ops if the ISR was 0, even before Kuogee's patch. That means that the only thing Kuogee's patch does is to prevent the call to "complete(>comp)" at the end of "dp_aux_isr()". ...and it makes sense not to call the complete() if no "isr" is 0. ...but what I'm saying is that _any_ non-zero value of ISR will still cause the complete() to be called after Kuogee's patch. That means that if any of the 32-bits in the "isr" variable are set, that we will call complete(). I'm asking if you're sure that every single bit of the "isr" means that we're ready to call complete(). It feels like it would be less fragile if dp_aux_native_handler() and dp_aux_i2c_handler() (which both already look at the ISR) returned some value saying whether the "isr" contained a bit that meant that complete() should be called. I'm almost certain I've asked for this before, but I can't find it anymore. Can we also simplify the aux handlers to be
Re: [Freedreno] [RFC PATCH 3/6] drm/msm/dpu1: Wire up DSC mask for active CTL configuration
On 14/12/2022 21:30, Marijn Suijten wrote:
On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
On 14/12/2022 01:22, Marijn Suijten wrote:
Active CTLs have to configure what DSC block(s) have to be enabled, and
what DSC block(s) have to be flushed; this value was initialized to zero
resulting in the necessary register writes to never happen (or would
write zero otherwise). This seems to have gotten lost in the DSC v4->v5
series while refactoring how the combination with merge_3d was handled.
Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
3 files changed, 4 insertions(+)
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 ae28b2b93e69..35791f93c33d 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
@@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
ctl->ops.setup_intf_cfg(ctl, _cfg);
/* setup which pp blk will connect to this intf */
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 0f71e8fe7be7..9ee3a7306a5f 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
@@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
intf_cfg.stream_sel = 0; /* Don't care value for video mode */
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
if (phys_enc->hw_pp->merge_3d)
intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 7cbcef6efe17..92ddf9995b37 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
dpu_encoder_phys *phys_enc)
intf_cfg.intf = DPU_NONE;
intf_cfg.wb = hw_wb->idx;
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
We usually don't have DSC with the writeback, don't we?
I am unsure so ended up adding them in writeback regardless. Downstream
uses a separate callback to process intf_cfg.dsc instead of going
through setup_intf_cfg().
To prevent these from being missed again (in the case of copy),
how about instead having some function that sets up intf_cfg with these
default values from a phys_enc? That way most of this remains oblivious
to the caller.
I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for
the WB.
On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
doesn't use the intf_cfg.dsc member anyway, but it was again added to
keep the blocks somewhat consistent (in case it ever becomes used?).
if (mode_3d && hw_pp && hw_pp->merge_3d)
intf_cfg.merge_3d = hw_pp->merge_3d->idx;
@@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
dpu_encoder_phys *phys_enc)
intf_cfg.wb = hw_wb->idx;
intf_cfg.mode_3d =
dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg);
}
}
- Marijn
--
With best wishes
Dmitry
Re: [Freedreno] [RFC PATCH 0/6] drm/msm: DSC Electric Boogaloo for sm8[12]50
On 14/12/2022 21:23, Marijn Suijten wrote: On 2022-12-14 20:40:06, Dmitry Baryshkov wrote: On 14/12/2022 01:22, Marijn Suijten wrote: This preliminary Display Stream Compression support package for (initially tested on) sm8[12]50 is based on comparing DSC behaviour between downstream and mainline. Some new callbacks are added (for binding blocks on active CTLs), logic bugs are corrected, zeroed struct members are now assigned proper values, and RM allocation and hw block retrieval now hand out (or not) DSC blocks without causing null-pointer dereferences. Unfortunately it is not yet enough to get rid of completely corrupted display output on the boards I tested here: - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels; - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz; - (can include more Xperia boards if desired) Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP and DSC, but only a single INTF/encoder/DSI-link. Hopefully this spawns some community/upstream interest to help rootcause our corruption issues (after we open a drm/msm report on GitLab for more appropriate tracking). The Sony Xperia XZ3 (sdm845) was fully tested and validated with this series to not cause any regressions (an one of the math fixes now allows us to change slice_count in the panel driver, which would corrupt previously). Marijn Suijten (6): drm/msm/dpu1: Implement DSC binding to PP block for CTL V1 drm/msm/dpu1: Add DSC config for sm8150 and sm8250 drm/msm/dpu1: Wire up DSC mask for active CTL configuration drm/msm/dsi: Use DSC slice(s) packet size to compute word count drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf drm/msm/dpu: Disallow unallocated (DSC) resources to be returned General comment: patches with Fixes ideally should come first. Usually they are picked into -fixes and/or stable kernels. If the Fixes patches are in the middle of the series, one can not be sure that they do not have dependencies on previous patches. If there is one, it should probably be stated clearly to ease work on backporting them. Ack, I may have rushed these RFC patches straight off my branches onto the lists in hopes of sparking some suggestions on what may still be broken or missing to get DSC working on sm[12]50, but will keep this in mind for v2 after receiving some more review. That said, any suggestions? From what I've noticed lately: - set dsc_version_major/dsc_version_minor - try using dsc params from 1.2 rater than 1.1 version spec (there is small difference there) -- With best wishes Dmitry
Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Quoting Kuogee Hsieh (2022-12-14 14:56:23)
>
> On 12/13/2022 3:06 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-12-13 13:44:05)
> >> Add both data-lanes and link-frequencies property into endpoint
> > Why do we care? Please tell us why it's important.
Any response?
> >> @@ -193,6 +217,8 @@ examples:
> >> reg = <1>;
> >> endpoint {
> >> remote-endpoint = <>;
> >> +data-lanes = <0 1>;
> >> +link-frequencies = /bits/ 64 <162000 27
> >> 54 81>;
> >> };
> > So far we haven't used the output port on the DP controller in DT.
> >
> > I'm still not clear on what we should do in general for DP because
> > there's a PHY that actually controls a lane count and lane mapping. In
> > my mental model of the SoC, this DP controller's output port is
> > connected to the DP PHY, which then sends the DP lanes out of the SoC to
> > the next downstream device (i.e. a DP connector or type-c muxer). Having
> > a remote-endpoint property with a phandle to typec doesn't fit my mental
> > model. I'd expect it to be the typec PHY.
> ack
> >
> > That brings up the question: when we have 2 lanes vs. 4 lanes will we
> > duplicate the data-lanes property in the PHY binding? I suspect we'll
> > have to. Hopefully that sort of duplication is OK?
> Current we have limitation by reserve 2 data lanes for usb2, i am not
> sure duplication to 4 lanes will work automatically.
> >
> > Similarly, we may have a redriver that limits the link-frequencies
> > property further (e.g. only support <= 2.7GHz). Having multiple
> > link-frequencies along the graph is OK, right? And isn't the
> > link-frequencies property known here by fact that the DP controller
> > tells us which SoC this controller is for, and thus we already know the
> > supported link frequencies?
> >
> > Finally, I wonder if we should put any of this in the DP controller's
> > output endpoint, or if we can put these sorts of properties in the DP
> > PHY binding directly? Can't we do that and then when the DP controller
> > tries to set 4 lanes, the PHY immediately fails the call and the link
> > training algorithm does its thing and tries fewer lanes? And similarly,
> > if link-frequencies were in the PHY's binding, the PHY could fail to set
> > those frequencies during link training, returning an error to the DP
> > controller, letting the training move on to a lower frequency. If we did
> > that this patch series would largely be about modifying the PHY binding,
> > updating the PHY driver to enforce constraints, and handling errors
> > during link training in the DP controller (which may already be done? I
> > didn't check).
>
>
> phy/pll have different configuration base on link lanes and rate.
>
> it has to be set up before link training can start.
>
> Once link training start, then there are no any interactions between
> controller and phy during link training session.
What do you mean? The DP controller calls phy_configure() and changes
the link rate. The return value from phy_configure() should be checked
and link training should skip link rates that aren't supported and/or
number of lanes that aren't supported.
>
> Link training only happen between dp controller and sink since link
> status is reported by sink (read back from sink's dpcd register directly).
>
> T achieve link symbol locked, link training will start from reduce link
> rate until lowest rate, if it still failed, then it will reduce lanes
> with highest rate and start training again.
>
> it will repeat same process until lowest lane (one lane), if it still
> failed, then it will give up and declare link training failed.
Yes, that describes the link training algorithm. I don't see why
phy_configure() return value can't be checked and either number of lanes
or link frequencies be checked. If only two lanes are supported, then
phy_configure() will fail for the 4 link rates and the algorithm will
reduce the number of lanes and go back to the highest rate. Then when
the highest rate isn't supported it will drop link rate until the link
rate is supported.
>
> Therefore I think add data-lanes and link-frequencies properties in the
> DP PHY binding directly will not helps.
>
I didn't follow your logic. Sorry.
Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi Doug On 12/14/2022 4:14 PM, Doug Anderson wrote: Hi, On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar wrote: Hi Doug On 12/14/2022 2:29 PM, Doug Anderson wrote: Hi, On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote: There are 3 possible interrupt sources are handled by DP controller, HPDstatus, Controller state changes and Aux read/write transaction. At every irq, DP controller have to check isr status of every interrupt sources and service the interrupt if its isr status bits shows interrupts are pending. There is potential race condition may happen at current aux isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() even irq is not for aux read or write transaction. This may cause aux read transaction return premature if host aux data read is in the middle of waiting for sink to complete transferring data to host while irq happen. This will cause host's receiving buffer contains unexpected data. This patch fixes this problem by checking aux isr and return immediately at aux isr handler if there are no any isr status bits set. Follows are the signature at kernel logs when problem happen, EDID has corrupt header panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index d030a93..8f8b12a 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) isr = dp_catalog_aux_get_irq(aux->catalog); + /* +* if this irq is not for aux transfer, +* then return immediately +*/ Why do you need 4 lines for a comment that fits on one line? Yes, we can fit this to one line. + if (!isr) + return; I can confirm that this works for me. I could reproduce the EDID problems in the past and I can't after this patch. ...so I could give a: Tested-by: Douglas Anderson I'm not an expert on this part of the code, so feel free to ignore my other comments if everyone else thinks this patch is fine as-is, but to me something here feels a little fragile. It feels a little weird that we'll "complete" for _any_ interrupt that comes through now rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() to specifically identify interrupts that caused the end of the transfer. I guess that idea is that every possible interrupt we get causes the end of the transfer? -Doug So this turned out to be more tricky and was a good finding from kuogee. In the bad EDID case, it was technically not bad EDID. What was happening was, the VIDEO_READY interrupt was continuously firing. Ideally, this should fire only once but due to some error condition it kept firing. We dont exactly know why yet what was the error condition making it continuously fire. In the DP ISR, the dp_aux_isr() gets called even if it was not an aux interrupt which fired (so the call flow in this case was dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() So we should certainly have some protection to return early from this routine if there was no aux interrupt which fired. Which is what this fix is doing. Its not completing any interrupt, its just returning early if no aux interrupt fired. ...but the whole problem was that it was doing the complete() at the end, right? Kuogee even mentioned that in the commit message. Specifically, I checked dp_aux_native_handler() and dp_aux_i2c_handler(), both of which are passed the "isr". Unless I messed up, both functions already were no-ops if the ISR was 0, even before Kuogee's patch. That means that the only thing Kuogee's patch does is to prevent the call to "complete(>comp)" at the end of "dp_aux_isr()". ...and it makes sense not to call the complete() if no "isr" is 0. ...but what I'm saying is that _any_ non-zero value of ISR will still cause the complete() to be called after Kuogee's patch. That means that if any of the 32-bits in the "isr" variable are set, that we will call complete(). I'm asking if you're sure that every single bit of the "isr" means that we're ready to call complete(). It feels like it would be less fragile if dp_aux_native_handler() and dp_aux_i2c_handler() (which both already look at the ISR) returned some value saying whether the "isr" contained a bit that meant that complete() should be called. Yes, so other than the "transfer done" bits, the other bits we listen to are below: 29 #define DP_INTERRUPT_STATUS1 \ 30 (DP_INTR_AUX_I2C_DONE| \ 31 DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ 32 DP_INTR_NACK_DEFER | DP_INTR_WRONG_DATA_CNT | \ 33 DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER | \ 34 DP_INTR_PLL_UNLOCKED | DP_INTR_AUX_ERROR All of these, if they fire, will be
Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Quoting Doug Anderson (2022-12-14 16:14:42) > Hi, > > On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar > wrote: > > > > Hi Doug > > > > On 12/14/2022 2:29 PM, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh > > > wrote: > > >> > > >> There are 3 possible interrupt sources are handled by DP controller, > > >> HPDstatus, Controller state changes and Aux read/write transaction. > > >> At every irq, DP controller have to check isr status of every interrupt > > >> sources and service the interrupt if its isr status bits shows interrupts > > >> are pending. There is potential race condition may happen at current aux > > >> isr handler implementation since it is always complete > > >> dp_aux_cmd_fifo_tx() > > >> even irq is not for aux read or write transaction. This may cause aux > > >> read > > >> transaction return premature if host aux data read is in the middle of > > >> waiting for sink to complete transferring data to host while irq happen. > > >> This will cause host's receiving buffer contains unexpected data. This > > >> patch fixes this problem by checking aux isr and return immediately at > > >> aux isr handler if there are no any isr status bits set. > > >> > > >> Follows are the signature at kernel logs when problem happen, > > >> EDID has corrupt header > > >> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID > > >> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel > > >> nor find a fallback > > >> > > >> Signed-off-by: Kuogee Hsieh > > >> --- > > >> drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ > > >> 1 file changed, 7 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c > > >> b/drivers/gpu/drm/msm/dp/dp_aux.c > > >> index d030a93..8f8b12a 100644 > > >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c > > >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > > >> @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > >> > > >> isr = dp_catalog_aux_get_irq(aux->catalog); > > >> > > >> + /* > > >> +* if this irq is not for aux transfer, > > >> +* then return immediately > > >> +*/ > > > > > > Why do you need 4 lines for a comment that fits on one line? > > Yes, we can fit this to one line. > > > > > >> + if (!isr) > > >> + return; > > > > > > I can confirm that this works for me. I could reproduce the EDID > > > problems in the past and I can't after this patch. ...so I could give > > > a: > > > > > > Tested-by: Douglas Anderson > > > > > > I'm not an expert on this part of the code, so feel free to ignore my > > > other comments if everyone else thinks this patch is fine as-is, but > > > to me something here feels a little fragile. It feels a little weird > > > that we'll "complete" for _any_ interrupt that comes through now > > > rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() > > > to specifically identify interrupts that caused the end of the > > > transfer. I guess that idea is that every possible interrupt we get > > > causes the end of the transfer? > > > > > > -Doug > > > > So this turned out to be more tricky and was a good finding from kuogee. > > > > In the bad EDID case, it was technically not bad EDID. > > > > What was happening was, the VIDEO_READY interrupt was continuously > > firing. Ideally, this should fire only once but due to some error > > condition it kept firing. We dont exactly know why yet what was the > > error condition making it continuously fire. This is a great detail that is missing from the commit text. > > > > In the DP ISR, the dp_aux_isr() gets called even if it was not an aux > > interrupt which fired (so the call flow in this case was > > dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() > > So we should certainly have some protection to return early from this > > routine if there was no aux interrupt which fired. I'm not sure that's a race condition though, more like a problem where the completion is called unconditionally? > > > > Which is what this fix is doing. > > > > Its not completing any interrupt, its just returning early if no aux > > interrupt fired. > > ...but the whole problem was that it was doing the complete() at the > end, right? Kuogee even mentioned that in the commit message. > Specifically, I checked dp_aux_native_handler() and > dp_aux_i2c_handler(), both of which are passed the "isr". Unless I > messed up, both functions already were no-ops if the ISR was 0, even > before Kuogee's patch. That means that the only thing Kuogee's patch > does is to prevent the call to "complete(>comp)" at the end of > "dp_aux_isr()". > > ...and it makes sense not to call the complete() if no "isr" is 0. > ...but what I'm saying is that _any_ non-zero value of ISR will still > cause the complete() to be called after Kuogee's patch. That means > that if any of the 32-bits in the "isr" variable are set, that we will > call complete(). I'm asking if you're
Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi, On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar wrote: > > Hi Doug > > On 12/14/2022 2:29 PM, Doug Anderson wrote: > > Hi, > > > > On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh > > wrote: > >> > >> There are 3 possible interrupt sources are handled by DP controller, > >> HPDstatus, Controller state changes and Aux read/write transaction. > >> At every irq, DP controller have to check isr status of every interrupt > >> sources and service the interrupt if its isr status bits shows interrupts > >> are pending. There is potential race condition may happen at current aux > >> isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() > >> even irq is not for aux read or write transaction. This may cause aux read > >> transaction return premature if host aux data read is in the middle of > >> waiting for sink to complete transferring data to host while irq happen. > >> This will cause host's receiving buffer contains unexpected data. This > >> patch fixes this problem by checking aux isr and return immediately at > >> aux isr handler if there are no any isr status bits set. > >> > >> Follows are the signature at kernel logs when problem happen, > >> EDID has corrupt header > >> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID > >> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor > >> find a fallback > >> > >> Signed-off-by: Kuogee Hsieh > >> --- > >> drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c > >> b/drivers/gpu/drm/msm/dp/dp_aux.c > >> index d030a93..8f8b12a 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > >> @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > >> > >> isr = dp_catalog_aux_get_irq(aux->catalog); > >> > >> + /* > >> +* if this irq is not for aux transfer, > >> +* then return immediately > >> +*/ > > > > Why do you need 4 lines for a comment that fits on one line? > Yes, we can fit this to one line. > > > >> + if (!isr) > >> + return; > > > > I can confirm that this works for me. I could reproduce the EDID > > problems in the past and I can't after this patch. ...so I could give > > a: > > > > Tested-by: Douglas Anderson > > > > I'm not an expert on this part of the code, so feel free to ignore my > > other comments if everyone else thinks this patch is fine as-is, but > > to me something here feels a little fragile. It feels a little weird > > that we'll "complete" for _any_ interrupt that comes through now > > rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() > > to specifically identify interrupts that caused the end of the > > transfer. I guess that idea is that every possible interrupt we get > > causes the end of the transfer? > > > > -Doug > > So this turned out to be more tricky and was a good finding from kuogee. > > In the bad EDID case, it was technically not bad EDID. > > What was happening was, the VIDEO_READY interrupt was continuously > firing. Ideally, this should fire only once but due to some error > condition it kept firing. We dont exactly know why yet what was the > error condition making it continuously fire. > > In the DP ISR, the dp_aux_isr() gets called even if it was not an aux > interrupt which fired (so the call flow in this case was > dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() > So we should certainly have some protection to return early from this > routine if there was no aux interrupt which fired. > > Which is what this fix is doing. > > Its not completing any interrupt, its just returning early if no aux > interrupt fired. ...but the whole problem was that it was doing the complete() at the end, right? Kuogee even mentioned that in the commit message. Specifically, I checked dp_aux_native_handler() and dp_aux_i2c_handler(), both of which are passed the "isr". Unless I messed up, both functions already were no-ops if the ISR was 0, even before Kuogee's patch. That means that the only thing Kuogee's patch does is to prevent the call to "complete(>comp)" at the end of "dp_aux_isr()". ...and it makes sense not to call the complete() if no "isr" is 0. ...but what I'm saying is that _any_ non-zero value of ISR will still cause the complete() to be called after Kuogee's patch. That means that if any of the 32-bits in the "isr" variable are set, that we will call complete(). I'm asking if you're sure that every single bit of the "isr" means that we're ready to call complete(). It feels like it would be less fragile if dp_aux_native_handler() and dp_aux_i2c_handler() (which both already look at the ISR) returned some value saying whether the "isr" contained a bit that meant that complete() should be called. -Doug
Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi Doug
On 12/14/2022 2:29 PM, Doug Anderson wrote:
Hi,
On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote:
There are 3 possible interrupt sources are handled by DP controller,
HPDstatus, Controller state changes and Aux read/write transaction.
At every irq, DP controller have to check isr status of every interrupt
sources and service the interrupt if its isr status bits shows interrupts
are pending. There is potential race condition may happen at current aux
isr handler implementation since it is always complete dp_aux_cmd_fifo_tx()
even irq is not for aux read or write transaction. This may cause aux read
transaction return premature if host aux data read is in the middle of
waiting for sink to complete transferring data to host while irq happen.
This will cause host's receiving buffer contains unexpected data. This
patch fixes this problem by checking aux isr and return immediately at
aux isr handler if there are no any isr status bits set.
Follows are the signature at kernel logs when problem happen,
EDID has corrupt header
panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID
panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find
a fallback
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index d030a93..8f8b12a 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
isr = dp_catalog_aux_get_irq(aux->catalog);
+ /*
+* if this irq is not for aux transfer,
+* then return immediately
+*/
Why do you need 4 lines for a comment that fits on one line?
Yes, we can fit this to one line.
+ if (!isr)
+ return;
I can confirm that this works for me. I could reproduce the EDID
problems in the past and I can't after this patch. ...so I could give
a:
Tested-by: Douglas Anderson
I'm not an expert on this part of the code, so feel free to ignore my
other comments if everyone else thinks this patch is fine as-is, but
to me something here feels a little fragile. It feels a little weird
that we'll "complete" for _any_ interrupt that comes through now
rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler()
to specifically identify interrupts that caused the end of the
transfer. I guess that idea is that every possible interrupt we get
causes the end of the transfer?
-Doug
So this turned out to be more tricky and was a good finding from kuogee.
In the bad EDID case, it was technically not bad EDID.
What was happening was, the VIDEO_READY interrupt was continuously
firing. Ideally, this should fire only once but due to some error
condition it kept firing. We dont exactly know why yet what was the
error condition making it continuously fire.
In the DP ISR, the dp_aux_isr() gets called even if it was not an aux
interrupt which fired (so the call flow in this case was
dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr()
So we should certainly have some protection to return early from this
routine if there was no aux interrupt which fired.
Which is what this fix is doing.
Its not completing any interrupt, its just returning early if no aux
interrupt fired.
So based on the logs I have seen and given that its fixing this
stability issue.
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Reviewed-by: Abhinav Kumar
Re: [Freedreno] [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
On 12/13/2022 3:06 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-12-13 13:44:05)
Add both data-lanes and link-frequencies property into endpoint
Why do we care? Please tell us why it's important.
Changes in v7:
-- split yaml out of dtsi patch
-- link-frequencies from link rate to symbol rate
-- deprecation of old data-lanes property
Changes in v8:
-- correct Bjorn mail address to kernel.org
Changes in v10:
-- add menu item to data-lanes and link-frequecnis
Changes in v11:
-- add endpoint property at port@1
Changes in v12:
-- use enum for item at data-lanes and link-frequencies
Signed-off-by: Kuogee Hsieh `
^
Stray ` here? -/
---
.../bindings/display/msm/dp-controller.yaml| 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index f2515af..8fb9fa5 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -96,14 +97,37 @@ properties:
ports:
$ref: /schemas/graph.yaml#/properties/ports
+
properties:
port@0:
-$ref: /schemas/graph.yaml#/properties/port
+$ref: "/schemas/graph.yaml#/$defs/port-base"
description: Input endpoint of the controller
+properties:
+ endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
port@1:
-$ref: /schemas/graph.yaml#/properties/port
+$ref: "/schemas/graph.yaml#/$defs/port-base"
I thought the quotes weren't needed?
description: Output endpoint of the controller
+properties:
+ endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
Does this need 'unevaluatedProperties: false' here?
+properties:
+ data-lanes:
+minItems: 1
+maxItems: 4
+items:
+ enum: [ 0, 1, 2, 3 ]
+
+ link-frequencies:
+minItems: 1
+maxItems: 4
+items:
+ enum: [ 162000, 27, 54, 81 ]
+
+required:
+ - port@0
+ - port@1
required:
- compatible
@@ -193,6 +217,8 @@ examples:
reg = <1>;
endpoint {
remote-endpoint = <>;
+data-lanes = <0 1>;
+link-frequencies = /bits/ 64 <162000 27
54 81>;
};
So far we haven't used the output port on the DP controller in DT.
I'm still not clear on what we should do in general for DP because
there's a PHY that actually controls a lane count and lane mapping. In
my mental model of the SoC, this DP controller's output port is
connected to the DP PHY, which then sends the DP lanes out of the SoC to
the next downstream device (i.e. a DP connector or type-c muxer). Having
a remote-endpoint property with a phandle to typec doesn't fit my mental
model. I'd expect it to be the typec PHY.
ack
That brings up the question: when we have 2 lanes vs. 4 lanes will we
duplicate the data-lanes property in the PHY binding? I suspect we'll
have to. Hopefully that sort of duplication is OK?
Current we have limitation by reserve 2 data lanes for usb2, i am not
sure duplication to 4 lanes will work automatically.
Similarly, we may have a redriver that limits the link-frequencies
property further (e.g. only support <= 2.7GHz). Having multiple
link-frequencies along the graph is OK, right? And isn't the
link-frequencies property known here by fact that the DP controller
tells us which SoC this controller is for, and thus we already know the
supported link frequencies?
Finally, I wonder if we should put any of this in the DP controller's
output endpoint, or if we can put these sorts of properties in the DP
PHY binding directly? Can't we do that and then when the DP controller
tries to set 4 lanes, the PHY immediately fails the call and the link
training algorithm does its thing and tries fewer lanes? And similarly,
if link-frequencies were in the PHY's binding, the PHY could fail to set
those frequencies during link training, returning an error to the DP
controller, letting the training move on to a lower frequency. If we did
that this patch series would largely be about modifying the PHY binding,
updating the PHY driver to enforce constraints, and handling errors
during link training in the DP controller (which may already be done? I
didn't check).
phy/pll have different configuration base on link lanes and rate.
it has to be set up before link training can start.
Once link training start, then there are no any interactions between
controller and phy during link training session.
Link training only happen between
Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi, On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote: > > There are 3 possible interrupt sources are handled by DP controller, > HPDstatus, Controller state changes and Aux read/write transaction. > At every irq, DP controller have to check isr status of every interrupt > sources and service the interrupt if its isr status bits shows interrupts > are pending. There is potential race condition may happen at current aux > isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() > even irq is not for aux read or write transaction. This may cause aux read > transaction return premature if host aux data read is in the middle of > waiting for sink to complete transferring data to host while irq happen. > This will cause host's receiving buffer contains unexpected data. This > patch fixes this problem by checking aux isr and return immediately at > aux isr handler if there are no any isr status bits set. > > Follows are the signature at kernel logs when problem happen, > EDID has corrupt header > panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID > panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor > find a fallback > > Signed-off-by: Kuogee Hsieh > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index d030a93..8f8b12a 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > isr = dp_catalog_aux_get_irq(aux->catalog); > > + /* > +* if this irq is not for aux transfer, > +* then return immediately > +*/ Why do you need 4 lines for a comment that fits on one line? > + if (!isr) > + return; I can confirm that this works for me. I could reproduce the EDID problems in the past and I can't after this patch. ...so I could give a: Tested-by: Douglas Anderson I'm not an expert on this part of the code, so feel free to ignore my other comments if everyone else thinks this patch is fine as-is, but to me something here feels a little fragile. It feels a little weird that we'll "complete" for _any_ interrupt that comes through now rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() to specifically identify interrupts that caused the end of the transfer. I guess that idea is that every possible interrupt we get causes the end of the transfer? -Doug
[Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
There are 3 possible interrupt sources are handled by DP controller, HPDstatus, Controller state changes and Aux read/write transaction. At every irq, DP controller have to check isr status of every interrupt sources and service the interrupt if its isr status bits shows interrupts are pending. There is potential race condition may happen at current aux isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() even irq is not for aux read or write transaction. This may cause aux read transaction return premature if host aux data read is in the middle of waiting for sink to complete transferring data to host while irq happen. This will cause host's receiving buffer contains unexpected data. This patch fixes this problem by checking aux isr and return immediately at aux isr handler if there are no any isr status bits set. Follows are the signature at kernel logs when problem happen, EDID has corrupt header panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index d030a93..8f8b12a 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) isr = dp_catalog_aux_get_irq(aux->catalog); + /* +* if this irq is not for aux transfer, +* then return immediately +*/ + if (!isr) + return; + if (!aux->cmd_busy) return; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [Freedreno] [v10] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280
On 2022-12-12 11:35:15, Kalyan Thota wrote: > [..] > >> + if (ctx->pending_dspp_flush_mask[dspp - DSPP_0]) > >> + DPU_REG_WRITE(>hw, CTL_DSPP_n_FLUSH(dspp - > >> DSPP_0), > >> + ctx->pending_dspp_flush_mask[dspp - > >> + DSPP_0]); > > > >Shouldn't this loop as a whole check if _any_ DSPP flush is requested via > >`pending_flush_mask & BIT(29)`? The other flushes don't check the per-block > >mask value either (and could write zero that way) but only base this check > >on the > >presence of a global flush mask for that block. > > > BIT(29) enables dspp flush only from DPU rev 7.x.x where hierarchal flush is > introduced. For other targets that supports CTL_ACTIVE, it's a NOP. The only way this patch ever writes pending_dspp_flush_mask is followed by unconditionally setting BIT(29) in pending_flush_mask. I was under the assumption that pending_dspp_flush_mask should be considered invalid or irrelevant unless BIT(29) is set. > With the check "pending_flush_mask & BIT(29)", unintended DSPP registers for > that CTL path will be programmed to "0" which is not correct IMO. You can also keep the second `if` to guard against that; as said the code above does exactly this though, but I think we could assume that if a pending sub-block flush is set, pending_dspp_flush_mask is nonzero? > Secondly "pending_flush_mask & BIT(29)" although will not be true for DPU > 6.x.x versions but can be confusing w.r.t code readability. > Let me know your thoughts. Ack, it is /super/ confusing that BIT(29) is used for DSPP (sub-block) flush, but also to flash INTF_2?? In fact there are many overlapping flush bits used for different components. Only few are clarified via a #define. Can you confirm whether this is correct? And whether these should all be pulled out into numerically-sorted defines to improve readability and document intentional overlap? - Marijn
Re: [Freedreno] [RFC PATCH 6/6] drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
On 2022-12-14 20:56:30, Dmitry Baryshkov wrote:
> On 14/12/2022 01:22, Marijn Suijten wrote:
> > In the event that the topology requests resources that have not been
> > created by the system (because they are typically not represented in
> > dpu_mdss_cfg ^1), the resource(s) in global_state (in this case DSC
> > blocks) remain NULL but will still be returned out of
> > dpu_rm_get_assigned_resources, where the caller expects to get an array
> > containing num_blks valid pointers (but instead gets these NULLs).
> >
> > To prevent this from happening, where null-pointer dereferences
> > typically result in a hard-to-debug platform lockup, num_blks shouldn't
> > increase past NULL blocks and will print an error and break instead.
> > After all, max_blks represents the static size of the maximum number of
> > blocks whereas the actual amount varies per platform.
> >
> > In the specific case of DSC initial resource allocation should behave
> > more like LMs and CTLs where NULL resources are skipped. The current
> > hardcoded mapping of DSC blocks should be loosened separately as DPU
> > 5.0.0 introduced a crossbar where DSC blocks can be "somewhat" freely
> > bound to any PP and CTL, but that hardcoding currently means that we
> > will return an error when the topology reserves a DSC that isn't
> > available, instead of looking for the next free one.
> >
> > ^1: which can happen after a git rebase ended up moving additions to
> > _dpu_cfg to a different struct which has the same patch context.
> >
> > Signed-off-by: Marijn Suijten
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > index 73b3442e7467..dcbf03d2940a 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > @@ -496,6 +496,11 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
> >
> > /* check if DSC required are allocated or not */
> > for (i = 0; i < num_dsc; i++) {
> > + if (!rm->dsc_blks[i]) {
> > + DPU_ERROR("DSC %d does not exist\n", i);
> > + return -EIO;
> > + }
> > +
> > if (global_state->dsc_to_enc_id[i]) {
> > DPU_ERROR("DSC %d is already allocated\n", i);
> > return -EIO;
> > @@ -660,6 +665,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
> > blks_size, enc_id);
> > break;
> > }
> > + if (!hw_blks[i]) {
> > + DPU_ERROR("No more resource %d available to assign to
> > enc %d\n",
> > + type, enc_id);
> > + break;
> > + }
> > blks[num_blks++] = hw_blks[i];
> > }
> >
>
> These two chunks should come as two separate patches, each having it's
> own Fixes tag.
Ack. They are indeed addressing different issues (with the same
outcome) with differing "backportability". Will address in v2, thanks
for pointing it out (and missing a Fixes: in the first place, of which
we already have so many...).
- Marijn
Re: [Freedreno] [RFC PATCH 3/6] drm/msm/dpu1: Wire up DSC mask for active CTL configuration
On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
> On 14/12/2022 01:22, Marijn Suijten wrote:
> > Active CTLs have to configure what DSC block(s) have to be enabled, and
> > what DSC block(s) have to be flushed; this value was initialized to zero
> > resulting in the necessary register writes to never happen (or would
> > write zero otherwise). This seems to have gotten lost in the DSC v4->v5
> > series while refactoring how the combination with merge_3d was handled.
> >
> > Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
> > Signed-off-by: Marijn Suijten
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
> > 3 files changed, 4 insertions(+)
> >
> > 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 ae28b2b93e69..35791f93c33d 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
> > @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
> > intf_cfg.stream_sel = cmd_enc->stream_sel;
> > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> > ctl->ops.setup_intf_cfg(ctl, _cfg);
> >
> > /* setup which pp blk will connect to this intf */
> > 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 0f71e8fe7be7..9ee3a7306a5f 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
> > @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
> > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
> > intf_cfg.stream_sel = 0; /* Don't care value for video mode */
> > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> > if (phys_enc->hw_pp->merge_3d)
> > intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > index 7cbcef6efe17..92ddf9995b37 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
> > dpu_encoder_phys *phys_enc)
> >
> > intf_cfg.intf = DPU_NONE;
> > intf_cfg.wb = hw_wb->idx;
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>
> We usually don't have DSC with the writeback, don't we?
I am unsure so ended up adding them in writeback regardless. Downstream
uses a separate callback to process intf_cfg.dsc instead of going
through setup_intf_cfg().
To prevent these from being missed again (in the case of copy),
how about instead having some function that sets up intf_cfg with these
default values from a phys_enc? That way most of this remains oblivious
to the caller.
On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
doesn't use the intf_cfg.dsc member anyway, but it was again added to
keep the blocks somewhat consistent (in case it ever becomes used?).
> > if (mode_3d && hw_pp && hw_pp->merge_3d)
> > intf_cfg.merge_3d = hw_pp->merge_3d->idx;
> > @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
> > dpu_encoder_phys *phys_enc)
> > intf_cfg.wb = hw_wb->idx;
> > intf_cfg.mode_3d =
> > dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> > phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
> > _cfg);
> > }
> > }
- Marijn
Re: [Freedreno] [RFC PATCH 0/6] drm/msm: DSC Electric Boogaloo for sm8[12]50
On 2022-12-14 20:40:06, Dmitry Baryshkov wrote: > On 14/12/2022 01:22, Marijn Suijten wrote: > > This preliminary Display Stream Compression support package for > > (initially tested on) sm8[12]50 is based on comparing DSC behaviour > > between downstream and mainline. Some new callbacks are added (for > > binding blocks on active CTLs), logic bugs are corrected, zeroed struct > > members are now assigned proper values, and RM allocation and hw block > > retrieval now hand out (or not) DSC blocks without causing null-pointer > > dereferences. > > > > Unfortunately it is not yet enough to get rid of completely corrupted > > display output on the boards I tested here: > > - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels; > > - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz; > > - (can include more Xperia boards if desired) > > > > Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP > > and DSC, but only a single INTF/encoder/DSI-link. > > > > Hopefully this spawns some community/upstream interest to help rootcause > > our corruption issues (after we open a drm/msm report on GitLab for more > > appropriate tracking). > > > > The Sony Xperia XZ3 (sdm845) was fully tested and validated with this > > series to not cause any regressions (an one of the math fixes now allows > > us to change slice_count in the panel driver, which would corrupt > > previously). > > > > Marijn Suijten (6): > >drm/msm/dpu1: Implement DSC binding to PP block for CTL V1 > >drm/msm/dpu1: Add DSC config for sm8150 and sm8250 > >drm/msm/dpu1: Wire up DSC mask for active CTL configuration > >drm/msm/dsi: Use DSC slice(s) packet size to compute word count > >drm/msm/dsi: Flip greater-than check for slice_count and > > slice_per_intf > >drm/msm/dpu: Disallow unallocated (DSC) resources to be returned > > General comment: patches with Fixes ideally should come first. Usually > they are picked into -fixes and/or stable kernels. If the Fixes patches > are in the middle of the series, one can not be sure that they do not > have dependencies on previous patches. If there is one, it should > probably be stated clearly to ease work on backporting them. Ack, I may have rushed these RFC patches straight off my branches onto the lists in hopes of sparking some suggestions on what may still be broken or missing to get DSC working on sm[12]50, but will keep this in mind for v2 after receiving some more review. That said, any suggestions? - Marijn
Re: [Freedreno] [RFC PATCH 6/6] drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
On 14/12/2022 01:22, Marijn Suijten wrote:
In the event that the topology requests resources that have not been
created by the system (because they are typically not represented in
dpu_mdss_cfg ^1), the resource(s) in global_state (in this case DSC
blocks) remain NULL but will still be returned out of
dpu_rm_get_assigned_resources, where the caller expects to get an array
containing num_blks valid pointers (but instead gets these NULLs).
To prevent this from happening, where null-pointer dereferences
typically result in a hard-to-debug platform lockup, num_blks shouldn't
increase past NULL blocks and will print an error and break instead.
After all, max_blks represents the static size of the maximum number of
blocks whereas the actual amount varies per platform.
In the specific case of DSC initial resource allocation should behave
more like LMs and CTLs where NULL resources are skipped. The current
hardcoded mapping of DSC blocks should be loosened separately as DPU
5.0.0 introduced a crossbar where DSC blocks can be "somewhat" freely
bound to any PP and CTL, but that hardcoding currently means that we
will return an error when the topology reserves a DSC that isn't
available, instead of looking for the next free one.
^1: which can happen after a git rebase ended up moving additions to
_dpu_cfg to a different struct which has the same patch context.
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 73b3442e7467..dcbf03d2940a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -496,6 +496,11 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
/* check if DSC required are allocated or not */
for (i = 0; i < num_dsc; i++) {
+ if (!rm->dsc_blks[i]) {
+ DPU_ERROR("DSC %d does not exist\n", i);
+ return -EIO;
+ }
+
if (global_state->dsc_to_enc_id[i]) {
DPU_ERROR("DSC %d is already allocated\n", i);
return -EIO;
@@ -660,6 +665,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
blks_size, enc_id);
break;
}
+ if (!hw_blks[i]) {
+ DPU_ERROR("No more resource %d available to assign to enc
%d\n",
+ type, enc_id);
+ break;
+ }
blks[num_blks++] = hw_blks[i];
}
These two chunks should come as two separate patches, each having it's
own Fixes tag.
--
With best wishes
Dmitry
Re: [Freedreno] [RFC PATCH 5/6] drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf
On 14/12/2022 01:22, Marijn Suijten wrote:
According to downstream /and the comment copied from it/ this comparison
should be the other way around. In other words, when the panel driver
requests to use more slices per packet than what could be sent over this
interface, it is bumped down to only use a single slice per packet (and
strangely not the number of slices that could fit on the interface).
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
With SoB in place:
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [RFC PATCH 4/6] drm/msm/dsi: Use DSC slice(s) packet size to compute word count
On 14/12/2022 01:22, Marijn Suijten wrote:
According to downstream the value to use for WORD_COUNT is
bytes_per_pkt, which denotes the number of bytes in a packet based on
how many slices have been configured by the panel driver times the
width of a slice times the number of bytes per pixel.
The DSC panels seen thus far use one byte per pixel, only one slice
per packet, and a slice width of half the panel width leading to the
desired bytes_per_pkt+1 value to be equal to hdisplay/2+1. This however
isn't the case anymore for panels that configure two slices per packet,
where the value should now be hdisplay+1.
Note that the aforementioned panel (on a Sony Xperia XZ3, sdm845) with
slice_count=1 has also been tested to successfully accept slice_count=2,
which would have shown corrupted output previously.
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [Freedreno] [RFC PATCH 3/6] drm/msm/dpu1: Wire up DSC mask for active CTL configuration
On 14/12/2022 01:22, Marijn Suijten wrote:
Active CTLs have to configure what DSC block(s) have to be enabled, and
what DSC block(s) have to be flushed; this value was initialized to zero
resulting in the necessary register writes to never happen (or would
write zero otherwise). This seems to have gotten lost in the DSC v4->v5
series while refactoring how the combination with merge_3d was handled.
Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++
3 files changed, 4 insertions(+)
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 ae28b2b93e69..35791f93c33d 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
@@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
intf_cfg.stream_sel = cmd_enc->stream_sel;
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
ctl->ops.setup_intf_cfg(ctl, _cfg);
/* setup which pp blk will connect to this intf */
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 0f71e8fe7be7..9ee3a7306a5f 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
@@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
intf_cfg.stream_sel = 0; /* Don't care value for video mode */
intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
if (phys_enc->hw_pp->merge_3d)
intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 7cbcef6efe17..92ddf9995b37 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
dpu_encoder_phys *phys_enc)
intf_cfg.intf = DPU_NONE;
intf_cfg.wb = hw_wb->idx;
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
We usually don't have DSC with the writeback, don't we?
if (mode_3d && hw_pp && hw_pp->merge_3d)
intf_cfg.merge_3d = hw_pp->merge_3d->idx;
@@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct
dpu_encoder_phys *phys_enc)
intf_cfg.wb = hw_wb->idx;
intf_cfg.mode_3d =
dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+ intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
_cfg);
}
}
--
With best wishes
Dmitry
Re: [Freedreno] [RFC PATCH 2/6] drm/msm/dpu1: Add DSC config for sm8150 and sm8250
On 14/12/2022 01:22, Marijn Suijten wrote: These blocks on CTL V1 support setting a PINGPONG idx to send pixel output to. Signed-off-by: Marijn Suijten --- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 23 ++- 1 file changed, 17 insertions(+), 6 deletions(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [Freedreno] [RFC PATCH 1/6] drm/msm/dpu1: Implement DSC binding to PP block for CTL V1
On 14/12/2022 01:22, Marijn Suijten wrote: All V1 CTL blocks (active CTLs) explicitly bind the pixel output from a DSC block to a PINGPONG block by setting the PINGPONG idx in a DSC hardware register. Signed-off-by: Marijn Suijten --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++ .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 9 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 27 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 4 +++ 4 files changed, 43 insertions(+) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [Freedreno] [RFC PATCH 0/6] drm/msm: DSC Electric Boogaloo for sm8[12]50
On 14/12/2022 01:22, Marijn Suijten wrote: This preliminary Display Stream Compression support package for (initially tested on) sm8[12]50 is based on comparing DSC behaviour between downstream and mainline. Some new callbacks are added (for binding blocks on active CTLs), logic bugs are corrected, zeroed struct members are now assigned proper values, and RM allocation and hw block retrieval now hand out (or not) DSC blocks without causing null-pointer dereferences. Unfortunately it is not yet enough to get rid of completely corrupted display output on the boards I tested here: - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels; - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz; - (can include more Xperia boards if desired) Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP and DSC, but only a single INTF/encoder/DSI-link. Hopefully this spawns some community/upstream interest to help rootcause our corruption issues (after we open a drm/msm report on GitLab for more appropriate tracking). The Sony Xperia XZ3 (sdm845) was fully tested and validated with this series to not cause any regressions (an one of the math fixes now allows us to change slice_count in the panel driver, which would corrupt previously). Marijn Suijten (6): drm/msm/dpu1: Implement DSC binding to PP block for CTL V1 drm/msm/dpu1: Add DSC config for sm8150 and sm8250 drm/msm/dpu1: Wire up DSC mask for active CTL configuration drm/msm/dsi: Use DSC slice(s) packet size to compute word count drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf drm/msm/dpu: Disallow unallocated (DSC) resources to be returned General comment: patches with Fixes ideally should come first. Usually they are picked into -fixes and/or stable kernels. If the Fixes patches are in the middle of the series, one can not be sure that they do not have dependencies on previous patches. If there is one, it should probably be stated clearly to ease work on backporting them. drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++ .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 + .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++ .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 23 +++- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 9 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 27 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 4 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 10 +++ drivers/gpu/drm/msm/dsi/dsi_host.c| 6 ++--- 10 files changed, 77 insertions(+), 9 deletions(-) -- 2.38.1 -- With best wishes Dmitry
Re: [Freedreno] [PATCH v13 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
On Tue, Dec 13, 2022 at 02:56:18PM -0800, Kuogee Hsieh wrote:
> Add both data-lanes and link-frequencies property into endpoint
>
> Changes in v7:
> -- split yaml out of dtsi patch
> -- link-frequencies from link rate to symbol rate
> -- deprecation of old data-lanes property
>
> Changes in v8:
> -- correct Bjorn mail address to kernel.org
>
> Changes in v10:
> -- add menu item to data-lanes and link-frequecnis
>
> Changes in v11:
> -- add endpoint property at port@1
>
> Changes in v12:
> -- use enum for item at data-lanes and link-frequencies
>
> Changes in v13:
> -- revised changes at port@0
> -- use correct ref schemas for both port@0 and port@1
> -- mark both port@0 and port@1 are required
> -- add line between data-lanes and link-frequencies properties
>
> Signed-off-by: Kuogee Hsieh `
> ---
> .../bindings/display/msm/dp-controller.yaml| 26
> --
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index f2515af..9d002de 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -81,6 +81,7 @@ properties:
>
>data-lanes:
> $ref: /schemas/types.yaml#/definitions/uint32-array
> +deprecated: true
> minItems: 1
> maxItems: 4
> items:
> @@ -98,12 +99,31 @@ properties:
> $ref: /schemas/graph.yaml#/properties/ports
> properties:
>port@0:
> -$ref: /schemas/graph.yaml#/properties/port
> +$ref: "/schemas/graph.yaml#/$defs/port-base"
This means you have extra properties to add in the endpoint, but you
didn't define any.
> description: Input endpoint of the controller
>
>port@1:
> -$ref: /schemas/graph.yaml#/properties/port
> +$ref: "/schemas/graph.yaml#/$defs/port-base"
Don't need quotes. Why did you add them?
> description: Output endpoint of the controller
> +properties:
> + endpoint:
> +$ref: /schemas/media/video-interfaces.yaml#
> +properties:
> + data-lanes:
> +minItems: 1
> +maxItems: 4
> +items:
> + enum: [ 0, 1, 2, 3 ]
> +
> + link-frequencies:
> +minItems: 1
> +maxItems: 4
> +items:
> + enum: [ 162000, 27, 54, 81 ]
> +
> +required:
> + - port@0
> + - port@1
>
> required:
>- compatible
> @@ -193,6 +213,8 @@ examples:
> reg = <1>;
> endpoint {
> remote-endpoint = <>;
> +data-lanes = <0 1>;
> +link-frequencies = /bits/ 64 <162000 27
> 54 81>;
> };
> };
> };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>
Re: [Freedreno] [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
On 14/12/2022 12:05, Vinod Polimera wrote:
Update crtc retrieval from dpu_enc to dpu_enc connector state,
since new links get set as part of the dpu enc virt mode set.
The dpu_enc->crtc cache is no more needed, hence cleaning it as
part of this change.
This patch is dependent on the series:
https://patchwork.freedesktop.org/series/110969/
Signed-off-by: Vinod Polimera
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 4 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 42 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 --
3 files changed, 13 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 3f72d38..289d51e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1029,7 +1029,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
*/
if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
release_bandwidth = true;
- dpu_encoder_assign_crtc(encoder, NULL);
}
/* wait for frame_event_done completion */
@@ -1099,9 +1098,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
dpu_crtc->enabled = true;
- drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
- dpu_encoder_assign_crtc(encoder, crtc);
-
/* Enable/restore vblank irq handling */
drm_crtc_vblank_on(crtc);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index a585036..b9b254d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -132,11 +132,6 @@ enum dpu_enc_rc_states {
* @intfs_swapped:Whether or not the phys_enc interfaces have been swapped
*for partial update right-only cases, such as pingpong
*split where virtual pingpong does not generate IRQs
- * @crtc: Pointer to the currently assigned crtc. Normally you
- * would use crtc->state->encoder_mask to determine the
- * link between encoder/crtc. However in this case we need
- * to track crtc in the disable() hook which is called
- * _after_ encoder_mask is cleared.
* @connector:If a mode is set, cached pointer to the active
connector
* @crtc_kickoff_cb: Callback into CRTC that will flush & start
*all CTL paths
@@ -181,7 +176,6 @@ struct dpu_encoder_virt {
bool intfs_swapped;
- struct drm_crtc *crtc;
struct drm_connector *connector;
struct dentry *debugfs_root;
@@ -1317,7 +1311,7 @@ static void dpu_encoder_vblank_callback(struct
drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
struct dpu_encoder_virt *dpu_enc = NULL;
- unsigned long lock_flags;
+ struct drm_crtc *crtc;
if (!drm_enc || !phy_enc)
return;
@@ -1325,12 +1319,13 @@ static void dpu_encoder_vblank_callback(struct
drm_encoder *drm_enc,
DPU_ATRACE_BEGIN("encoder_vblank_callback");
dpu_enc = to_dpu_encoder_virt(drm_enc);
- atomic_inc(_enc->vsync_cnt);
+ if (!dpu_enc->connector || !dpu_enc->connector->state ||
+ !dpu_enc->connector->state->crtc)
+ return;
- spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
- if (dpu_enc->crtc)
- dpu_crtc_vblank_callback(dpu_enc->crtc);
- spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags);
+ atomic_inc(_enc->vsync_cnt);
+ crtc = dpu_enc->connector->state->crtc;
+ dpu_crtc_vblank_callback(crtc);
DPU_ATRACE_END("encoder_vblank_callback");
}
@@ -1353,33 +1348,22 @@ static void dpu_encoder_underrun_callback(struct
drm_encoder *drm_enc,
DPU_ATRACE_END("encoder_underrun_callback");
}
-void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc)
-{
- struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
- unsigned long lock_flags;
-
- spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
- /* crtc should always be cleared before re-assigning */
- WARN_ON(crtc && dpu_enc->crtc);
- dpu_enc->crtc = crtc;
- spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags);
-}
-
void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
struct drm_crtc *crtc, bool enable)
{
struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
- unsigned long lock_flags;
+ struct drm_crtc *new_crtc;
int i;
trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
- spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
- if (dpu_enc->crtc != crtc) {
-
[Freedreno] [PATCH v9 15/15] drm/msm/disp/dpu: clear active interface in the datapath cleanup
Clear interface active register from the datapath for a clean shutdown of the datapath. Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index a3ba696..bdff09c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -2098,6 +2098,9 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) if (phys_enc->hw_pp->merge_3d) intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx; + if (phys_enc->hw_intf) + intf_cfg.intf = phys_enc->hw_intf->idx; + if (ctl->ops.reset_intf_cfg) ctl->ops.reset_intf_cfg(ctl, _cfg); -- 2.7.4
[Freedreno] [PATCH v9 14/15] drm/msm/disp/dpu: reset the datapath after timing engine disable
Reset the datapath after disabling the timing gen, such that it can start on a clean slate when the intf is enabled back. This was a recommended sequence from the DPU HW programming guide. Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + 1 file changed, 1 insertion(+) 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 685cb44..a597cca 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 @@ -591,6 +591,7 @@ static void dpu_encoder_phys_vid_disable(struct dpu_encoder_phys *phys_enc) } } + dpu_encoder_helper_phys_cleanup(phys_enc); phys_enc->enable_state = DPU_ENC_DISABLED; } -- 2.7.4
[Freedreno] [PATCH v9 13/15] drm/msm/disp/dpu: wait for extra vsync till timing engine status is disabled
There can be a race between timing gen disable and vblank irq. The
wait post timing gen disable may return early but intf disable sequence
might not be completed. Ensure that, intf status is disabled before
we retire the function.
Signed-off-by: Vinod Polimera
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c| 21 +
1 file changed, 21 insertions(+)
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 0f71e8f..685cb44 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
@@ -526,6 +526,7 @@ static void dpu_encoder_phys_vid_disable(struct
dpu_encoder_phys *phys_enc)
{
unsigned long lock_flags;
int ret;
+ struct intf_status intf_status = {0};
if (!phys_enc->parent || !phys_enc->parent->dev) {
DPU_ERROR("invalid encoder/device\n");
@@ -570,6 +571,26 @@ static void dpu_encoder_phys_vid_disable(struct
dpu_encoder_phys *phys_enc)
}
}
+ if (phys_enc->hw_intf && phys_enc->hw_intf->ops.get_status)
+ phys_enc->hw_intf->ops.get_status(phys_enc->hw_intf,
_status);
+
+ /*
+* Wait for a vsync if timing en status is on after timing engine
+* is disabled.
+*/
+ if (intf_status.is_en && dpu_encoder_phys_vid_is_master(phys_enc)) {
+ spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
+ dpu_encoder_phys_inc_pending(phys_enc);
+ spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
+ ret = dpu_encoder_phys_vid_wait_for_vblank(phys_enc);
+ if (ret) {
+ atomic_set(_enc->pending_kickoff_cnt, 0);
+ DRM_ERROR("wait disable failed: id:%u intf:%d ret:%d\n",
+ DRMID(phys_enc->parent),
+ phys_enc->hw_intf->idx - INTF_0, ret);
+ }
+ }
+
phys_enc->enable_state = DPU_ENC_DISABLED;
}
--
2.7.4
[Freedreno] [PATCH v9 12/15] drm/msm/disp/dpu: get timing engine status from intf status register
Recommended way of reading the interface timing gen status is via
status register. Timing gen status register will give a reliable status
of the interface especially during ON/OFF transitions. This support was
added from DPU version 5.0.0.
Signed-off-by: Vinod Polimera
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 12 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 8 +++-
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 2196e20..0e410cd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -77,7 +77,8 @@
#define INTF_SC7180_MASK BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE)
-#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
+#define INTF_SC7280_MASK \
+ (INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) |
BIT(DPU_INTF_STATUS_SUPPORTED))
#define IRQ_SDM845_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
BIT(MDP_SSPP_TOP0_INTR2) | \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 3b645d5..b16b428 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -204,17 +204,19 @@ enum {
/**
* INTF sub-blocks
- * @DPU_INTF_INPUT_CTRL Supports the setting of pp block from which
- * pixel data arrives to this INTF
- * @DPU_INTF_TE INTF block has TE configuration support
- * @DPU_DATA_HCTL_ENAllows data to be transferred at different rate
-than video timing
+ * @DPU_INTF_INPUT_CTRL Supports the setting of pp block from which
+ * pixel data arrives to this INTF
+ * @DPU_INTF_TE INTF block has TE configuration support
+ * @DPU_DATA_HCTL_ENAllows data to be transferred at different
rate
+ * than video timing
+ * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
* @DPU_INTF_MAX
*/
enum {
DPU_INTF_INPUT_CTRL = 0x1,
DPU_INTF_TE,
DPU_DATA_HCTL_EN,
+ DPU_INTF_STATUS_SUPPORTED,
DPU_INTF_MAX
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 7ce66bf..84ee2ef 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -62,6 +62,7 @@
#define INTF_LINE_COUNT 0x0B0
#define INTF_MUX 0x25C
+#define INTF_STATUS 0x26C
#define INTF_CFG_ACTIVE_H_EN BIT(29)
#define INTF_CFG_ACTIVE_V_EN BIT(30)
@@ -297,8 +298,13 @@ static void dpu_hw_intf_get_status(
struct intf_status *s)
{
struct dpu_hw_blk_reg_map *c = >hw;
+ unsigned long cap = intf->cap->features;
+
+ if (cap & BIT(DPU_INTF_STATUS_SUPPORTED))
+ s->is_en = DPU_REG_READ(c, INTF_STATUS) & BIT(0);
+ else
+ s->is_en = DPU_REG_READ(c, INTF_TIMING_ENGINE_EN);
- s->is_en = DPU_REG_READ(c, INTF_TIMING_ENGINE_EN);
s->is_prog_fetch_en = !!(DPU_REG_READ(c, INTF_CONFIG) & BIT(31));
if (s->is_en) {
s->frame_count = DPU_REG_READ(c, INTF_FRAME_COUNT);
--
2.7.4
[Freedreno] [PATCH v9 11/15] drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
Enable PSR on eDP interface using drm self-refresh librabry.
This patch uses a trigger from self-refresh library to enter/exit
into PSR, when there are no updates from framework.
Signed-off-by: Kalyan Thota
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 13 -
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
3 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 289d51e..386b037 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include "dpu_kms.h"
#include "dpu_hw_lm.h"
@@ -1018,6 +1019,9 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
+ if (old_crtc_state->self_refresh_active)
+ return;
+
/* Disable/save vblank irq handling */
drm_crtc_vblank_off(crtc);
@@ -1572,7 +1576,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev,
struct drm_plane *plane,
{
struct drm_crtc *crtc = NULL;
struct dpu_crtc *dpu_crtc = NULL;
- int i;
+ int i, ret;
dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
if (!dpu_crtc)
@@ -1610,6 +1614,13 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev,
struct drm_plane *plane,
/* initialize event handling */
spin_lock_init(_crtc->event_lock);
+ ret = drm_self_refresh_helper_init(crtc);
+ if (ret) {
+ DPU_ERROR("Failed to initialize %s with self-refresh helpers
%d\n",
+ crtc->name, ret);
+ return ERR_PTR(ret);
+ }
+
DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name);
return crtc;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index dbf0d96..a3ba696 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
#include
@@ -1237,11 +1238,24 @@ static void dpu_encoder_virt_atomic_disable(struct
drm_encoder *drm_enc,
struct drm_atomic_state *state)
{
struct dpu_encoder_virt *dpu_enc = NULL;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_state = NULL;
int i = 0;
dpu_enc = to_dpu_encoder_virt(drm_enc);
DPU_DEBUG_ENC(dpu_enc, "\n");
+ crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc);
+ if (crtc)
+ old_state = drm_atomic_get_old_crtc_state(state, crtc);
+
+ /*
+* The encoder is already disabled if self refresh mode was set earlier,
+* in the old_state for the corresponding crtc.
+*/
+ if (old_state && old_state->self_refresh_active)
+ return;
+
mutex_lock(_enc->enc_lock);
dpu_enc->enabled = false;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 93221a2..a969874 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -491,7 +491,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms
*kms,
return;
}
- if (!crtc->state->active) {
+ if (!drm_atomic_crtc_effectively_active(crtc->state)) {
DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id);
return;
}
--
2.7.4
[Freedreno] [PATCH v9 10/15] drm/msm/disp/dpu: check for crtc enable rather than crtc active to release shared resources
According to KMS documentation, The driver must not release any shared
resources if active is set to false but enable still true.
Fixes: ccc862b957c6 ("drm/msm/dpu: Fix reservation failures in modeset")
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index a0cb089..dbf0d96 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -677,7 +677,7 @@ static int dpu_encoder_virt_atomic_check(
if (drm_atomic_crtc_needs_modeset(crtc_state)) {
dpu_rm_release(global_state, drm_enc);
- if (!crtc_state->active_changed || crtc_state->active)
+ if (!crtc_state->active_changed || crtc_state->enable)
ret = dpu_rm_reserve(_kms->rm, global_state,
drm_enc, crtc_state, topology);
}
--
2.7.4
[Freedreno] [PATCH v9 08/15] drm/bridge: add psr support for panel bridge callbacks
This change will handle the psr entry exit cases in the panel
bridge atomic callback functions. For example, the panel power
should not turn off if the panel is entering psr.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
---
drivers/gpu/drm/bridge/panel.c | 48 ++
1 file changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 3558cbf..5e77e38 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -113,6 +113,18 @@ static void panel_bridge_atomic_pre_enable(struct
drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+
+ crtc = drm_atomic_get_new_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(atomic_state, crtc);
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
drm_panel_prepare(panel_bridge->panel);
}
@@ -121,6 +133,18 @@ static void panel_bridge_atomic_enable(struct drm_bridge
*bridge,
struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+
+ crtc = drm_atomic_get_new_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(atomic_state, crtc);
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
drm_panel_enable(panel_bridge->panel);
}
@@ -129,6 +153,18 @@ static void panel_bridge_atomic_disable(struct drm_bridge
*bridge,
struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state;
+
+ crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
+ if (new_crtc_state && new_crtc_state->self_refresh_active)
+ return;
drm_panel_disable(panel_bridge->panel);
}
@@ -137,6 +173,18 @@ static void panel_bridge_atomic_post_disable(struct
drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state;
+
+ crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
+ if (new_crtc_state && new_crtc_state->self_refresh_active)
+ return;
drm_panel_unprepare(panel_bridge->panel);
}
--
2.7.4
[Freedreno] [PATCH v9 09/15] drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder functions
Use atomic variants for encoder callback functions such that
certain states like self-refresh can be accessed as part of
enable/disable sequence.
Signed-off-by: Kalyan Thota
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b9b254d..a0cb089 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1196,7 +1196,8 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder
*drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
+static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
+ struct drm_atomic_state *state)
{
struct dpu_encoder_virt *dpu_enc = NULL;
int ret = 0;
@@ -1232,7 +1233,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder
*drm_enc)
mutex_unlock(_enc->enc_lock);
}
-static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
+static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
+ struct drm_atomic_state *state)
{
struct dpu_encoder_virt *dpu_enc = NULL;
int i = 0;
@@ -2407,8 +2409,8 @@ static void dpu_encoder_frame_done_timeout(struct
timer_list *t)
static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
- .disable = dpu_encoder_virt_disable,
- .enable = dpu_encoder_virt_enable,
+ .atomic_disable = dpu_encoder_virt_atomic_disable,
+ .atomic_enable = dpu_encoder_virt_atomic_enable,
.atomic_check = dpu_encoder_virt_atomic_check,
};
--
2.7.4
[Freedreno] [PATCH v9 07/15] drm/bridge: use atomic enable/disable callbacks for panel bridge
Use atomic variants for panel bridge callback functions such that
certain states like self-refresh can be accessed as part of
enable/disable sequence.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/bridge/panel.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 216af76..3558cbf 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -109,28 +109,32 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
drm_connector_cleanup(connector);
}
-static void panel_bridge_pre_enable(struct drm_bridge *bridge)
+static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
drm_panel_prepare(panel_bridge->panel);
}
-static void panel_bridge_enable(struct drm_bridge *bridge)
+static void panel_bridge_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
drm_panel_enable(panel_bridge->panel);
}
-static void panel_bridge_disable(struct drm_bridge *bridge)
+static void panel_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
drm_panel_disable(panel_bridge->panel);
}
-static void panel_bridge_post_disable(struct drm_bridge *bridge)
+static void panel_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
@@ -159,10 +163,10 @@ static void panel_bridge_debugfs_init(struct drm_bridge
*bridge,
static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
.attach = panel_bridge_attach,
.detach = panel_bridge_detach,
- .pre_enable = panel_bridge_pre_enable,
- .enable = panel_bridge_enable,
- .disable = panel_bridge_disable,
- .post_disable = panel_bridge_post_disable,
+ .atomic_pre_enable = panel_bridge_atomic_pre_enable,
+ .atomic_enable = panel_bridge_atomic_enable,
+ .atomic_disable = panel_bridge_atomic_disable,
+ .atomic_post_disable = panel_bridge_atomic_post_disable,
.get_modes = panel_bridge_get_modes,
.atomic_reset = drm_atomic_helper_bridge_reset,
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
--
2.7.4
[Freedreno] [PATCH v9 06/15] drm/msm/dp: disable self_refresh_aware after entering psr
From: Sankeerth Billakanti
Updated frames get queued if self_refresh_aware is set when the
sink is in psr. To support bridge enable and avoid queuing of update
frames, reset the self_refresh_aware state after entering psr.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
---
drivers/gpu/drm/msm/dp/dp_drm.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index d3e9010..0f262a6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -131,6 +131,8 @@ static void edp_bridge_atomic_enable(struct drm_bridge
*drm_bridge,
struct drm_crtc_state *old_crtc_state;
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state = NULL;
/*
* Check the old state of the crtc to determine if the panel
@@ -147,10 +149,22 @@ static void edp_bridge_atomic_enable(struct drm_bridge
*drm_bridge,
if (old_crtc_state && old_crtc_state->self_refresh_active) {
dp_display_set_psr(dp, false);
- return;
+ goto psr_aware;
}
dp_bridge_atomic_enable(drm_bridge, old_bridge_state);
+
+psr_aware:
+ connector = drm_atomic_get_new_connector_for_encoder(atomic_state,
+ drm_bridge->encoder);
+ if (connector)
+ conn_state = drm_atomic_get_new_connector_state(atomic_state,
+ connector);
+
+ if (conn_state) {
+ conn_state->self_refresh_aware = dp->psr_supported;
+ }
+
}
static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
@@ -161,6 +175,14 @@ static void edp_bridge_atomic_disable(struct drm_bridge
*drm_bridge,
struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state = NULL;
+
+ connector = drm_atomic_get_old_connector_for_encoder(atomic_state,
+ drm_bridge->encoder);
+ if (connector)
+ conn_state = drm_atomic_get_new_connector_state(atomic_state,
+ connector);
crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state,
drm_bridge->encoder);
@@ -187,6 +209,9 @@ static void edp_bridge_atomic_disable(struct drm_bridge
*drm_bridge,
* when display disable occurs while the sink is in psr state.
*/
if (new_crtc_state->self_refresh_active) {
+ if (conn_state)
+ conn_state->self_refresh_aware = false;
+
dp_display_set_psr(dp, true);
return;
} else if (old_crtc_state->self_refresh_active) {
--
2.7.4
[Freedreno] [PATCH v9 05/15] drm/msm/dp: use the eDP bridge ops to validate eDP modes
The eDP and DP interfaces shared the bridge operations and
the eDP specific changes were implemented under is_edp check.
To add psr support for eDP, we started using a new set of eDP
bridge ops. We are moving the eDP specific code in the
dp_bridge_mode_valid function to a new eDP function,
edp_bridge_mode_valid under the eDP bridge ops.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 8
drivers/gpu/drm/msm/dp/dp_drm.c | 34 +-
2 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 7ec81b8..91642a0 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -984,14 +984,6 @@ enum drm_mode_status dp_bridge_mode_valid(struct
drm_bridge *bridge,
return -EINVAL;
}
- /*
-* The eDP controller currently does not have a reliable way of
-* enabling panel power to read sink capabilities. So, we rely
-* on the panel driver to populate only supported modes for now.
-*/
- if (dp->is_edp)
- return MODE_OK;
-
if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
return MODE_CLOCK_HIGH;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index df66df0..d3e9010 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -223,12 +223,44 @@ static void edp_bridge_atomic_post_disable(struct
drm_bridge *drm_bridge,
dp_bridge_atomic_post_disable(drm_bridge, old_bridge_state);
}
+/**
+ * edp_bridge_mode_valid - callback to determine if specified mode is valid
+ * @bridge: Pointer to drm bridge structure
+ * @info: display info
+ * @mode: Pointer to drm mode structure
+ * Returns: Validity status for specified mode
+ */
+static enum drm_mode_status edp_bridge_mode_valid(struct drm_bridge *bridge,
+ const struct drm_display_info *info,
+ const struct drm_display_mode *mode)
+{
+ struct msm_dp *dp;
+ int mode_pclk_khz = mode->clock;
+
+ dp = to_dp_bridge(bridge)->dp_display;
+
+ if (!dp || !mode_pclk_khz || !dp->connector) {
+ DRM_ERROR("invalid params\n");
+ return -EINVAL;
+ }
+
+ if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
+ return MODE_CLOCK_HIGH;
+
+ /*
+* The eDP controller currently does not have a reliable way of
+* enabling panel power to read sink capabilities. So, we rely
+* on the panel driver to populate only supported modes for now.
+*/
+ return MODE_OK;
+}
+
static const struct drm_bridge_funcs edp_bridge_ops = {
.atomic_enable = edp_bridge_atomic_enable,
.atomic_disable = edp_bridge_atomic_disable,
.atomic_post_disable = edp_bridge_atomic_post_disable,
.mode_set = dp_bridge_mode_set,
- .mode_valid = dp_bridge_mode_valid,
+ .mode_valid = edp_bridge_mode_valid,
.atomic_reset = drm_atomic_helper_bridge_reset,
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
--
2.7.4
[Freedreno] [PATCH v9 04/15] drm/msm/dp: Add basic PSR support for eDP
Add support for basic panel self refresh (PSR) feature for eDP.
Add a new interface to set PSR state in the sink from DPU.
Program the eDP controller to issue PSR enter and exit SDP to
the sink.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 80 ++
drivers/gpu/drm/msm/dp/dp_catalog.h | 4 ++
drivers/gpu/drm/msm/dp/dp_ctrl.c| 80 ++
drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 +
drivers/gpu/drm/msm/dp/dp_display.c | 19 ++
drivers/gpu/drm/msm/dp/dp_display.h | 2 +
drivers/gpu/drm/msm/dp/dp_drm.c | 133 +++-
drivers/gpu/drm/msm/dp/dp_link.c| 36 ++
drivers/gpu/drm/msm/dp/dp_panel.c | 22 ++
drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
drivers/gpu/drm/msm/dp/dp_reg.h | 27
11 files changed, 411 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 676279d..c12a5d9 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -47,6 +47,14 @@
#define DP_INTERRUPT_STATUS2_MASK \
(DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT)
+#define DP_INTERRUPT_STATUS4 \
+ (PSR_UPDATE_INT | PSR_CAPTURE_INT | PSR_EXIT_INT | \
+ PSR_UPDATE_ERROR_INT | PSR_WAKE_ERROR_INT)
+
+#define DP_INTERRUPT_MASK4 \
+ (PSR_UPDATE_MASK | PSR_CAPTURE_MASK | PSR_EXIT_MASK | \
+ PSR_UPDATE_ERROR_MASK | PSR_WAKE_ERROR_MASK)
+
struct dp_catalog_private {
struct device *dev;
struct drm_device *drm_dev;
@@ -359,6 +367,23 @@ void dp_catalog_ctrl_lane_mapping(struct dp_catalog
*dp_catalog)
ln_mapping);
}
+void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog,
+ bool enable)
+{
+ u32 val;
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ val = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
+
+ if (enable)
+ val |= DP_MAINLINK_CTRL_ENABLE;
+ else
+ val &= ~DP_MAINLINK_CTRL_ENABLE;
+
+ dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val);
+}
+
void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
bool enable)
{
@@ -610,6 +635,47 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
*dp_catalog)
dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
}
+static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog)
+{
+ /* trigger sdp */
+ dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP);
+ dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0);
+}
+
+void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+ u32 config;
+
+ /* enable PSR1 function */
+ config = dp_read_link(catalog, REG_PSR_CONFIG);
+ config |= PSR1_SUPPORTED;
+ dp_write_link(catalog, REG_PSR_CONFIG, config);
+
+ dp_write_ahb(catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4);
+ dp_catalog_enable_sdp(catalog);
+}
+
+void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+ u32 cmd;
+
+ cmd = dp_read_link(catalog, REG_PSR_CMD);
+
+ cmd &= ~(PSR_ENTER | PSR_EXIT);
+
+ if (enter)
+ cmd |= PSR_ENTER;
+ else
+ cmd |= PSR_EXIT;
+
+ dp_catalog_enable_sdp(catalog);
+ dp_write_link(catalog, REG_PSR_CMD, cmd);
+}
+
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
@@ -645,6 +711,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog
*dp_catalog)
return isr & (mask | ~DP_DP_HPD_INT_MASK);
}
+u32 dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog *dp_catalog)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+ u32 intr, intr_ack;
+
+ intr = dp_read_ahb(catalog, REG_DP_INTR_STATUS4);
+ intr_ack = (intr & DP_INTERRUPT_STATUS4)
+ << DP_INTERRUPT_STATUS_ACK_SHIFT;
+ dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack);
+
+ return intr;
+}
+
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 1f717f4..2174bb5 100644
---
[Freedreno] [PATCH v9 03/15] drm/msm/dp: use atomic callbacks for DP bridge ops
Use atomic variants for DP bridge callback functions so that
the atomic state can be accessed in the interface drivers.
The atomic state will help the driver find out if the display
is in self refresh state.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Douglas Anderson
---
drivers/gpu/drm/msm/dp/dp_display.c | 9 ++---
drivers/gpu/drm/msm/dp/dp_drm.c | 16
drivers/gpu/drm/msm/dp/dp_drm.h | 9 ++---
3 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 7ff60e5..c04a02b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1643,7 +1643,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct
drm_device *dev,
return 0;
}
-void dp_bridge_enable(struct drm_bridge *drm_bridge)
+void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
+struct drm_bridge_state *old_bridge_state)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
@@ -1698,7 +1699,8 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
mutex_unlock(_display->event_mutex);
}
-void dp_bridge_disable(struct drm_bridge *drm_bridge)
+void dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
@@ -1709,7 +1711,8 @@ void dp_bridge_disable(struct drm_bridge *drm_bridge)
dp_ctrl_push_idle(dp_display->ctrl);
}
-void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
+void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 6db82f9..77c34cb 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -94,14 +94,14 @@ static const struct drm_bridge_funcs dp_bridge_ops = {
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
.atomic_reset = drm_atomic_helper_bridge_reset,
- .enable = dp_bridge_enable,
- .disable = dp_bridge_disable,
- .post_disable = dp_bridge_post_disable,
- .mode_set = dp_bridge_mode_set,
- .mode_valid = dp_bridge_mode_valid,
- .get_modes= dp_bridge_get_modes,
- .detect = dp_bridge_detect,
- .atomic_check = dp_bridge_atomic_check,
+ .atomic_enable = dp_bridge_atomic_enable,
+ .atomic_disable = dp_bridge_atomic_disable,
+ .atomic_post_disable= dp_bridge_atomic_post_disable,
+ .mode_set = dp_bridge_mode_set,
+ .mode_valid = dp_bridge_mode_valid,
+ .get_modes = dp_bridge_get_modes,
+ .detect = dp_bridge_detect,
+ .atomic_check = dp_bridge_atomic_check,
};
struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device
*dev,
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index 82035db..e69c0b9 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -23,9 +23,12 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp
*dp_display, struct dr
struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device
*dev,
struct drm_encoder *encoder);
-void dp_bridge_enable(struct drm_bridge *drm_bridge);
-void dp_bridge_disable(struct drm_bridge *drm_bridge);
-void dp_bridge_post_disable(struct drm_bridge *drm_bridge);
+void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
+struct drm_bridge_state *old_bridge_state);
+void dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state);
+void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state);
enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
const struct drm_display_info *info,
const struct drm_display_mode *mode);
--
2.7.4
[Freedreno] [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
Update crtc retrieval from dpu_enc to dpu_enc connector state,
since new links get set as part of the dpu enc virt mode set.
The dpu_enc->crtc cache is no more needed, hence cleaning it as
part of this change.
This patch is dependent on the series:
https://patchwork.freedesktop.org/series/110969/
Signed-off-by: Vinod Polimera
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 4 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 42 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 --
3 files changed, 13 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 3f72d38..289d51e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1029,7 +1029,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
*/
if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
release_bandwidth = true;
- dpu_encoder_assign_crtc(encoder, NULL);
}
/* wait for frame_event_done completion */
@@ -1099,9 +1098,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
dpu_crtc->enabled = true;
- drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
- dpu_encoder_assign_crtc(encoder, crtc);
-
/* Enable/restore vblank irq handling */
drm_crtc_vblank_on(crtc);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index a585036..b9b254d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -132,11 +132,6 @@ enum dpu_enc_rc_states {
* @intfs_swapped: Whether or not the phys_enc interfaces have been swapped
* for partial update right-only cases, such as pingpong
* split where virtual pingpong does not generate IRQs
- * @crtc: Pointer to the currently assigned crtc. Normally you
- * would use crtc->state->encoder_mask to determine the
- * link between encoder/crtc. However in this case we need
- * to track crtc in the disable() hook which is called
- * _after_ encoder_mask is cleared.
* @connector: If a mode is set, cached pointer to the active connector
* @crtc_kickoff_cb: Callback into CRTC that will flush & start
* all CTL paths
@@ -181,7 +176,6 @@ struct dpu_encoder_virt {
bool intfs_swapped;
- struct drm_crtc *crtc;
struct drm_connector *connector;
struct dentry *debugfs_root;
@@ -1317,7 +1311,7 @@ static void dpu_encoder_vblank_callback(struct
drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
struct dpu_encoder_virt *dpu_enc = NULL;
- unsigned long lock_flags;
+ struct drm_crtc *crtc;
if (!drm_enc || !phy_enc)
return;
@@ -1325,12 +1319,13 @@ static void dpu_encoder_vblank_callback(struct
drm_encoder *drm_enc,
DPU_ATRACE_BEGIN("encoder_vblank_callback");
dpu_enc = to_dpu_encoder_virt(drm_enc);
- atomic_inc(_enc->vsync_cnt);
+ if (!dpu_enc->connector || !dpu_enc->connector->state ||
+ !dpu_enc->connector->state->crtc)
+ return;
- spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
- if (dpu_enc->crtc)
- dpu_crtc_vblank_callback(dpu_enc->crtc);
- spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags);
+ atomic_inc(_enc->vsync_cnt);
+ crtc = dpu_enc->connector->state->crtc;
+ dpu_crtc_vblank_callback(crtc);
DPU_ATRACE_END("encoder_vblank_callback");
}
@@ -1353,33 +1348,22 @@ static void dpu_encoder_underrun_callback(struct
drm_encoder *drm_enc,
DPU_ATRACE_END("encoder_underrun_callback");
}
-void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc
*crtc)
-{
- struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
- unsigned long lock_flags;
-
- spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
- /* crtc should always be cleared before re-assigning */
- WARN_ON(crtc && dpu_enc->crtc);
- dpu_enc->crtc = crtc;
- spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags);
-}
-
void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
struct drm_crtc *crtc, bool enable)
{
struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
- unsigned long lock_flags;
+ struct drm_crtc *new_crtc;
int i;
trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
- spin_lock_irqsave(_enc->enc_spinlock, lock_flags);
- if (dpu_enc->crtc != crtc) {
-
[Freedreno] [PATCH v9 02/15] drm: add helper functions to retrieve old and new crtc
Add new helper functions, drm_atomic_get_old_crtc_for_encoder
and drm_atomic_get_new_crtc_for_encoder to retrieve the
corresponding crtc for the encoder.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
Reviewed-by: Douglas Anderson
---
drivers/gpu/drm/drm_atomic.c | 60
include/drm/drm_atomic.h | 7 ++
2 files changed, 67 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f197f59..941fd6d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -985,6 +985,66 @@ drm_atomic_get_new_connector_for_encoder(struct
drm_atomic_state *state,
EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
/**
+ * drm_atomic_get_old_crtc_for_encoder - Get old crtc for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the crtc state for
+ *
+ * This function finds and returns the crtc that was connected to @encoder
+ * as specified by the @state.
+ *
+ * Returns: The old crtc connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_crtc *
+drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder)
+{
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_old_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+EXPORT_SYMBOL(drm_atomic_get_old_crtc_for_encoder);
+
+/**
+ * drm_atomic_get_new_crtc_for_encoder - Get new crtc for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the crtc state for
+ *
+ * This function finds and returns the crtc that will be connected to @encoder
+ * as specified by the @state.
+ *
+ * Returns: The new crtc connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_crtc *
+drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder)
+{
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_encoder);
+
+/**
* drm_atomic_get_connector_state - get connector state
* @state: global atomic state object
* @connector: connector to get state object for
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 10b1990..fdbd656 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -528,6 +528,13 @@ struct drm_connector *
drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state,
struct drm_encoder *encoder);
+struct drm_crtc *
+drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state,
+struct drm_encoder *encoder);
+struct drm_crtc *
+drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_state *state,
+struct drm_encoder *encoder);
+
/**
* drm_atomic_get_existing_crtc_state - get CRTC state, if it exists
* @state: global atomic state object
--
2.7.4
[Freedreno] [PATCH v9 00/15] Add PSR support for eDP
Changes in v2: - Use dp bridge to set psr entry/exit instead of dpu_enocder. - Don't modify whitespaces. - Set self refresh aware from atomic_check. - Set self refresh aware only if psr is supported. - Provide a stub for msm_dp_display_set_psr. - Move dp functions to bridge code. Changes in v3: - Change callback names to reflect atomic interfaces. - Move bridge callback change to separate patch as suggested by Dmitry. - Remove psr function declaration from msm_drv.h. - Set self_refresh_aware flag only if psr is supported. - Modify the variable names to simpler form. - Define bit fields for PSR settings. - Add comments explaining the steps to enter/exit psr. - Change DRM_INFO to drm_dbg_db. Changes in v4: - Move the get crtc functions to drm_atomic. - Add atomic functions for DP bridge too. - Add ternary operator to choose eDP or DP ops. - Return true/false instead of 1/0. - mode_valid missing in the eDP bridge ops. - Move the functions to get crtc into drm_atomic.c. - Fix compilation issues. - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc. - Check for crtc state enable while reserving resources. Changes in v5: - Move the mode_valid changes into a different patch. - Complete psr_op_comp only when isr is set. - Move the DP atomic callback changes to a different patch. - Get crtc from drm connector state crtc. - Move to separate patch for check for crtc state enable while reserving resources. Changes in v6: - Remove crtc from dpu_encoder_virt struct. - fix crtc check during vblank toggle crtc. - Misc changes. Changes in v7: - Add fix for underrun issue on kasan build. Changes in v8: - Drop the enc spinlock as it won't serve any purpose in protetcing conn state.(Dmitry/Doug) Changes in v9: - Update commit message and fix alignment using spaces.(Marijn) - Misc changes.(Marijn) Sankeerth Billakanti (1): drm/msm/dp: disable self_refresh_aware after entering psr Vinod Polimera (14): drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc drm: add helper functions to retrieve old and new crtc drm/msm/dp: use atomic callbacks for DP bridge ops drm/msm/dp: Add basic PSR support for eDP drm/msm/dp: use the eDP bridge ops to validate eDP modes drm/bridge: use atomic enable/disable callbacks for panel bridge drm/bridge: add psr support for panel bridge callbacks drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder functions drm/msm/disp/dpu: check for crtc enable rather than crtc active to release shared resources drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver drm/msm/disp/dpu: get timing engine status from intf status register drm/msm/disp/dpu: wait for extra vsync till timing engine status is disabled drm/msm/disp/dpu: reset the datapath after timing engine disable drm/msm/disp/dpu: clear active interface in the datapath cleanup drivers/gpu/drm/bridge/panel.c | 68 ++- drivers/gpu/drm/drm_atomic.c | 60 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 +- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 71 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 8 - .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 22 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 12 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 8 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 2 +- drivers/gpu/drm/msm/dp/dp_catalog.c| 80 drivers/gpu/drm/msm/dp/dp_catalog.h| 4 + drivers/gpu/drm/msm/dp/dp_ctrl.c | 80 drivers/gpu/drm/msm/dp/dp_ctrl.h | 3 + drivers/gpu/drm/msm/dp/dp_display.c| 36 ++-- drivers/gpu/drm/msm/dp/dp_display.h| 2 + drivers/gpu/drm/msm/dp/dp_drm.c| 206 - drivers/gpu/drm/msm/dp/dp_drm.h| 9 +- drivers/gpu/drm/msm/dp/dp_link.c | 36 drivers/gpu/drm/msm/dp/dp_panel.c | 22 +++ drivers/gpu/drm/msm/dp/dp_panel.h | 6 + drivers/gpu/drm/msm/dp/dp_reg.h| 27 +++ include/drm/drm_atomic.h | 7 + 23 files changed, 703 insertions(+), 86 deletions(-) -- 2.7.4
Re: [Freedreno] [PATCH v2 01/12] dt-bindings: display: msm: Rename mdss node name in example
14 декабря 2022 г. 00:11:58 GMT+02:00, Dmitry Baryshkov пишет: > > >On 13 December 2022 23:53:48 EET, Abhinav Kumar >wrote: >> >> >>On 12/1/2022 11:54 AM, Dmitry Baryshkov wrote: >>> On 30/11/2022 22:09, Adam Skladowski wrote: >> >>> >>> We will pick this into msm-fixes during the next cycle. >> >>Yes, we can with the above fixes tags but first, can you please send a MR >>from msm-next-lumag to msm-next? So that I can send a MR for fixes to >>msm-next. > >This would create an additional merge commit in msm-next for no particular >reason. You can branch -fixes from rc1, or from the msm-next-lumag and then >send MR to msm-next. Another option would be to base msm-fixes on drm-next directly. > >> >>ATM, they are out of sync. >> >>
Re: [Freedreno] [RFC PATCH 5/6] drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf
On 2022-12-14 01:02:14, Konrad Dybcio wrote:
>
>
> On 14.12.2022 00:22, Marijn Suijten wrote:
> > According to downstream /and the comment copied from it/ this comparison
> > should be the other way around. In other words, when the panel driver
> > requests to use more slices per packet than what could be sent over this
> > interface, it is bumped down to only use a single slice per packet (and
> > strangely not the number of slices that could fit on the interface).
> >
> > Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Marijn Suijten
> > ---
> Missing s-o-b
Thanks for catching, checkpatch would've pointed this out (in addition
to a typo in the cover letter) if I had ran it.
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 0686c35a6fd4..9bdfa0864cdf 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -855,11 +855,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> > *msm_host, bool is_cmd_mod
> > */
> > slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
> >
> > - /* If slice_per_pkt is greater than slice_per_intf
> > + /* If slice_count is greater than slice_per_intf
> > * then default to 1. This can happen during partial
> > * update.
> > */
> > - if (slice_per_intf > dsc->slice_count)
> > + if (dsc->slice_count > slice_per_intf)
> > dsc->slice_count = 1;
> >
> > total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
