| Message ID | 1681247095-1201-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: always program dsc active bits | expand |
On 4/11/2023 2:04 PM, Kuogee Hsieh wrote: > In current code, the dsc active bits are set only if the cfg->dsc is set. > However, for displays which are hot-pluggable, there can be a use-case > of disconnecting a DSC supported sink and connecting a non-DSC sink. > > For those cases we need to clear DSC active bits during teardown. > > Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") Again, wrong fixes tag, Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > index bbdc95c..88e4efe 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if (cfg->merge_3d) > DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > BIT(cfg->merge_3d - MERGE_3D_0)); > - if (cfg->dsc) { > - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > - } > + > + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > } > > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, But, otherwise seems fine and a valid bug fix. Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Full-caps DSC in the title, as discussed previously. On that note, don't forget to CC those who have reviewed your patches previously, as also brought up in earlier review. On 2023-04-11 14:04:55, Kuogee Hsieh wrote: > In current code, the dsc active bits are set only if the cfg->dsc is set. Some typo nits: DSC* active bits. s/are set/are written/ (the variable is set, registers are written). Drop `the` before `cfg->dsc` (and you could replace `s/is set/is non-zero/). > However, for displays which are hot-pluggable, there can be a use-case > of disconnecting a DSC supported sink and connecting a non-DSC sink. > > For those cases we need to clear DSC active bits during teardown. > > Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> If you have validated that it is fine to write these registers on _every_ platform supported by DPU1, and after fixing the above nits and the Fixes: commit hash as pointed out by Abhinav: Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> And see one question below. > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > index bbdc95c..88e4efe 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if (cfg->merge_3d) > DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > BIT(cfg->merge_3d - MERGE_3D_0)); > - if (cfg->dsc) { > - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > - } > + > + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); Does this flush all DSCs programmed in CTL_DSC_FLUSH as set above? That is currently still in `if (cfg->dsc)` and never overwritten if all DSCs are disabled, should it be taken out of the `if` to make sure no DSCs are inadvertently flushed, or otherwise cache the "previous mask" to make sure we flush exactly the right DSC blocks? Thanks! - Marijn > + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > } > > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 12/04/2023 00:04, Kuogee Hsieh wrote: > In current code, the dsc active bits are set only if the cfg->dsc is set. > However, for displays which are hot-pluggable, there can be a use-case > of disconnecting a DSC supported sink and connecting a non-DSC sink. > > For those cases we need to clear DSC active bits during teardown. Please correct me if I'm wrong here, shouldn't we start using reset_intf_cfg() during teardown / unplug? > > Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > index bbdc95c..88e4efe 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c > @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, > if (cfg->merge_3d) > DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, > BIT(cfg->merge_3d - MERGE_3D_0)); > - if (cfg->dsc) { > - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > - } > + > + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); > } > > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
On 4/11/2023 3:17 PM, Dmitry Baryshkov wrote: > On 12/04/2023 00:04, Kuogee Hsieh wrote: >> In current code, the dsc active bits are set only if the cfg->dsc is set. >> However, for displays which are hot-pluggable, there can be a use-case >> of disconnecting a DSC supported sink and connecting a non-DSC sink. >> >> For those cases we need to clear DSC active bits during teardown. > > Please correct me if I'm wrong here, shouldn't we start using > reset_intf_cfg() during teardown / unplug? > This is actually a good point. Since PSR landed this cycle, we are doing dpu_encoder_helper_phys_cleanup() even for video mode path, 22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine disable") I was doing it only for writeback path as I had not validated video mode enough with the dpu_encoder_helper_phys_cleanup() API. But looking closely, I think there is an issue with the flush logic in that API for video mode. The reset API, calls a ctl->ops.trigger_flush(ctl); but its getting called after timing engine turns off today so this wont take any effect. We need to improve that API and add the missing pieces for it to work correctly with video mode and re-validate the issue for which PSR made that change. So needs more work there. This change works because the timing engine is enabled right after this call and will trigger the flush with it. The only drawback of this change is DSC_ACTIVE will always get written to either with 0 or the right value but only once during enable. I think this change is fine till we finish the rest of the pieces. We can add the if (cfg->dsc) back to this when we fix the reset_intf_cfg() to handle DSC and dpu_encoder_helper_phys_cleanup() to handle flush correctly. I will take up that work. >> >> Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> index bbdc95c..88e4efe 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct >> dpu_hw_ctl *ctx, >> if (cfg->merge_3d) >> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >> BIT(cfg->merge_3d - MERGE_3D_0)); >> - if (cfg->dsc) { >> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >> - } >> + >> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >> } >> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >
On 12/04/2023 02:32, Abhinav Kumar wrote: > > > On 4/11/2023 3:17 PM, Dmitry Baryshkov wrote: >> On 12/04/2023 00:04, Kuogee Hsieh wrote: >>> In current code, the dsc active bits are set only if the cfg->dsc is >>> set. >>> However, for displays which are hot-pluggable, there can be a use-case >>> of disconnecting a DSC supported sink and connecting a non-DSC sink. >>> >>> For those cases we need to clear DSC active bits during teardown. >> >> Please correct me if I'm wrong here, shouldn't we start using >> reset_intf_cfg() during teardown / unplug? >> > > This is actually a good point. Since PSR landed this cycle, we are doing > dpu_encoder_helper_phys_cleanup() even for video mode path, > > 22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine > disable") > > I was doing it only for writeback path as I had not validated video mode > enough with the dpu_encoder_helper_phys_cleanup() API. > > But looking closely, I think there is an issue with the flush logic in > that API for video mode. > > The reset API, calls a ctl->ops.trigger_flush(ctl); but its getting > called after timing engine turns off today so this wont take any effect. > > We need to improve that API and add the missing pieces for it to work > correctly with video mode and re-validate the issue for which PSR made > that change. So needs more work there. > > This change works because the timing engine is enabled right after this > call and will trigger the flush with it. > > The only drawback of this change is DSC_ACTIVE will always get written > to either with 0 or the right value but only once during enable. > > I think this change is fine till we finish the rest of the pieces. We > can add the if (cfg->dsc) back to this when we fix the reset_intf_cfg() > to handle DSC and dpu_encoder_helper_phys_cleanup() to handle flush > correctly. I'd agree here. Then a FIXME comment would be nice. > > I will take up that work. > >>> >>> Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") >>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> index bbdc95c..88e4efe 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >>> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct >>> dpu_hw_ctl *ctx, >>> if (cfg->merge_3d) >>> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >>> BIT(cfg->merge_3d - MERGE_3D_0)); >>> - if (cfg->dsc) { >>> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >>> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >>> - } >>> + >>> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >>> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >>> } >>> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >>
On 4/11/2023 3:14 PM, Marijn Suijten wrote: > Full-caps DSC in the title, as discussed previously. > > On that note, don't forget to CC those who have reviewed your patches > previously, as also brought up in earlier review. > > On 2023-04-11 14:04:55, Kuogee Hsieh wrote: >> In current code, the dsc active bits are set only if the cfg->dsc is set. > > Some typo nits: > > DSC* active bits. > > s/are set/are written/ (the variable is set, registers are written). > > Drop `the` before `cfg->dsc` (and you could replace `s/is set/is > non-zero/). > >> However, for displays which are hot-pluggable, there can be a use-case >> of disconnecting a DSC supported sink and connecting a non-DSC sink. >> >> For those cases we need to clear DSC active bits during teardown. >> >> Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > > If you have validated that it is fine to write these registers on > _every_ platform supported by DPU1, and after fixing the above nits and > the Fixes: commit hash as pointed out by Abhinav: > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > And see one question below. > >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> index bbdc95c..88e4efe 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c >> @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, >> if (cfg->merge_3d) >> DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, >> BIT(cfg->merge_3d - MERGE_3D_0)); >> - if (cfg->dsc) { >> - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); >> - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >> - } >> + >> + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); > > Does this flush all DSCs programmed in CTL_DSC_FLUSH as set above? That > is currently still in `if (cfg->dsc)` and never overwritten if all DSCs > are disabled, should it be taken out of the `if` to make sure no DSCs > are inadvertently flushed, or otherwise cache the "previous mask" to > make sure we flush exactly the right DSC blocks? > Yes, DSC flush is hierarchical. This is the main DSC flush which will enforce the flush of the DSC's we are trying to flush in the CTL_DSC_FLUSH register. So if DSC was active, the CTL_FLUSH will only enforce the flush of the DSC's programmed in CTL_DSC_FLUSH If DSC is not active, we still need to flush that as well (that was the missing bit). No need to cache previous mask. That programming should be accurate in cfg->dsc already. > Thanks! > > - Marijn > >> + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); >> } >> >> static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx, >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >>
On 2023-04-11 16:45:34, Abhinav Kumar wrote: [..] > > Does this flush all DSCs programmed in CTL_DSC_FLUSH as set above? That > > is currently still in `if (cfg->dsc)` and never overwritten if all DSCs > > are disabled, should it be taken out of the `if` to make sure no DSCs > > are inadvertently flushed, or otherwise cache the "previous mask" to > > make sure we flush exactly the right DSC blocks? > > > > Yes, DSC flush is hierarchical. This is the main DSC flush which will > enforce the flush of the DSC's we are trying to flush in the > CTL_DSC_FLUSH register. That's what I was thinking, thanks for confirming. > So if DSC was active, the CTL_FLUSH will only enforce the flush of the > DSC's programmed in CTL_DSC_FLUSH > > If DSC is not active, we still need to flush that as well (that was the > missing bit). > > No need to cache previous mask. That programming should be accurate in > cfg->dsc already. This kind of implicit dependency warrants a comment at the very least. What happens if a device boots without DSC panel connected? Will CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the DSC blocks? Or could this flush uninitialized state to the block? - Marijn
On 4/12/2023 12:24 AM, Marijn Suijten wrote: > On 2023-04-11 16:45:34, Abhinav Kumar wrote: > [..] >>> Does this flush all DSCs programmed in CTL_DSC_FLUSH as set above? That >>> is currently still in `if (cfg->dsc)` and never overwritten if all DSCs >>> are disabled, should it be taken out of the `if` to make sure no DSCs >>> are inadvertently flushed, or otherwise cache the "previous mask" to >>> make sure we flush exactly the right DSC blocks? >>> >> >> Yes, DSC flush is hierarchical. This is the main DSC flush which will >> enforce the flush of the DSC's we are trying to flush in the >> CTL_DSC_FLUSH register. > > That's what I was thinking, thanks for confirming. > >> So if DSC was active, the CTL_FLUSH will only enforce the flush of the >> DSC's programmed in CTL_DSC_FLUSH >> >> If DSC is not active, we still need to flush that as well (that was the >> missing bit). >> >> No need to cache previous mask. That programming should be accurate in >> cfg->dsc already. > > This kind of implicit dependency warrants a comment at the very least. > Sure. > What happens if a device boots without DSC panel connected? Will > CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the > DSC blocks? Or could this flush uninitialized state to the block? > If we bootup without DSC panel connected, the kernel's cfg->dsc will be 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush any DSC blocks. Sure, as I wrote in the other response, we can move this to reset_intf_cfg later when the other pieces are fixed. And leave a FIXME here. > - Marijn
On 2023-04-12 10:33:15, Abhinav Kumar wrote: [..] > > What happens if a device boots without DSC panel connected? Will > > CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the > > DSC blocks? Or could this flush uninitialized state to the block? > > > > If we bootup without DSC panel connected, the kernel's cfg->dsc will be > 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush > any DSC blocks. Ack, that makes sense. However, if I connect a DSC panel, then disconnect it (now the register should be non-zero, but cfg->dsc will be zero), and then replug a non-DSC panel multiple times, it'll get flushed every time because we never clear CTL_DSC_FLUSH after that? > Sure, as I wrote in the other response, we can move this > to reset_intf_cfg later when the other pieces are fixed. And leave a > FIXME here. Kuogee forgot to CC me on this patchs so I did not read/receive that side of the email thread. Will catch up before reviewing v2. - Marijn
On 4/14/2023 12:35 AM, Marijn Suijten wrote: > On 2023-04-12 10:33:15, Abhinav Kumar wrote: > [..] >>> What happens if a device boots without DSC panel connected? Will >>> CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the >>> DSC blocks? Or could this flush uninitialized state to the block? >>> >> >> If we bootup without DSC panel connected, the kernel's cfg->dsc will be >> 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush >> any DSC blocks. > > Ack, that makes sense. However, if I connect a DSC panel, then > disconnect it (now the register should be non-zero, but cfg->dsc will be > zero), and then replug a non-DSC panel multiple times, it'll get flushed > every time because we never clear CTL_DSC_FLUSH after that? > If we remove it after kernel starts, that issue is there even today without that change because DSI is not a hot-pluggable display so a teardown wont happen when you plug out the panel. How will cfg->dsc be 0 then? In that case, its not a valid test as there was no indication to DRM that display was disconnected so we cannot tear it down. >> Sure, as I wrote in the other response, we can move this >> to reset_intf_cfg later when the other pieces are fixed. And leave a >> FIXME here. > > Kuogee forgot to CC me on this patchs so I did not read/receive that > side of the email thread. Will catch up before reviewing v2. > > - Marijn
On 2023-04-14 08:48:43, Abhinav Kumar wrote: > > On 4/14/2023 12:35 AM, Marijn Suijten wrote: > > On 2023-04-12 10:33:15, Abhinav Kumar wrote: > > [..] > >>> What happens if a device boots without DSC panel connected? Will > >>> CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the > >>> DSC blocks? Or could this flush uninitialized state to the block? > >>> > >> > >> If we bootup without DSC panel connected, the kernel's cfg->dsc will be > >> 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush > >> any DSC blocks. > > > > Ack, that makes sense. However, if I connect a DSC panel, then > > disconnect it (now the register should be non-zero, but cfg->dsc will be > > zero), and then replug a non-DSC panel multiple times, it'll get flushed > > every time because we never clear CTL_DSC_FLUSH after that? > > > > If we remove it after kernel starts, that issue is there even today > without that change because DSI is not a hot-pluggable display so a > teardown wont happen when you plug out the panel. How will cfg->dsc be 0 > then? In that case, its not a valid test as there was no indication to > DRM that display was disconnected so we cannot tear it down. The patch description itself describes hot-pluggable displays, which I believe is the upcoming DSC support for DP? You ask how cfg->dsc can become zero, but this is **exactly** what the patch description describes, and what this patch is removing the `if` for. If we are not allowed to discuss that scenario because it is not currently supported, neither should we allow to apply this patch. With that in mind, can you re-answer the question? - Marijn
On 4/14/2023 10:34 AM, Marijn Suijten wrote: > On 2023-04-14 08:48:43, Abhinav Kumar wrote: >> >> On 4/14/2023 12:35 AM, Marijn Suijten wrote: >>> On 2023-04-12 10:33:15, Abhinav Kumar wrote: >>> [..] >>>>> What happens if a device boots without DSC panel connected? Will >>>>> CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the >>>>> DSC blocks? Or could this flush uninitialized state to the block? >>>>> >>>> >>>> If we bootup without DSC panel connected, the kernel's cfg->dsc will be >>>> 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush >>>> any DSC blocks. >>> >>> Ack, that makes sense. However, if I connect a DSC panel, then >>> disconnect it (now the register should be non-zero, but cfg->dsc will be >>> zero), and then replug a non-DSC panel multiple times, it'll get flushed >>> every time because we never clear CTL_DSC_FLUSH after that? >>> >> >> If we remove it after kernel starts, that issue is there even today >> without that change because DSI is not a hot-pluggable display so a >> teardown wont happen when you plug out the panel. How will cfg->dsc be 0 >> then? In that case, its not a valid test as there was no indication to >> DRM that display was disconnected so we cannot tear it down. > > The patch description itself describes hot-pluggable displays, which I > believe is the upcoming DSC support for DP? You ask how cfg->dsc can > become zero, but this is **exactly** what the patch description > describes, and what this patch is removing the `if` for. If we are not > allowed to discuss that scenario because it is not currently supported, > neither should we allow to apply this patch. > > With that in mind, can you re-answer the question? > I didnt follow what needs to be re-answered. This patch is being sent in preparation of the DSC over DP support. This does not handle non-hotpluggable displays. I do not think dynamic switch between DSC and non-DSC of non-hotpluggable displays needs to be discussed here as its not handled at all with or without this patch. We wanted to get early reviews on the patch. If you want this patch to be absorbed when rest of DSC over DP lands, I have no concerns with that. I wont pick this up for fixes and we will land this together with the rest of DP over DSC. > - Marijn
On 2023-04-14 10:57:45, Abhinav Kumar wrote: > On 4/14/2023 10:34 AM, Marijn Suijten wrote: > > On 2023-04-14 08:48:43, Abhinav Kumar wrote: > >> On 4/14/2023 12:35 AM, Marijn Suijten wrote: > >>> On 2023-04-12 10:33:15, Abhinav Kumar wrote: > >>> [..] > >>>>> What happens if a device boots without DSC panel connected? Will > >>>>> CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the > >>>>> DSC blocks? Or could this flush uninitialized state to the block? > >>>> > >>>> If we bootup without DSC panel connected, the kernel's cfg->dsc will be > >>>> 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush > >>>> any DSC blocks. > >>> > >>> Ack, that makes sense. However, if I connect a DSC panel, then > >>> disconnect it (now the register should be non-zero, but cfg->dsc will be > >>> zero), and then replug a non-DSC panel multiple times, it'll get flushed > >>> every time because we never clear CTL_DSC_FLUSH after that? > >> > >> If we remove it after kernel starts, that issue is there even today > >> without that change because DSI is not a hot-pluggable display so a > >> teardown wont happen when you plug out the panel. How will cfg->dsc be 0 > >> then? In that case, its not a valid test as there was no indication to > >> DRM that display was disconnected so we cannot tear it down. > > > > The patch description itself describes hot-pluggable displays, which I > > believe is the upcoming DSC support for DP? You ask how cfg->dsc can > > become zero, but this is **exactly** what the patch description > > describes, and what this patch is removing the `if` for. If we are not > > allowed to discuss that scenario because it is not currently supported, > > neither should we allow to apply this patch. > > > > With that in mind, can you re-answer the question? > > I didnt follow what needs to be re-answered. > > This patch is being sent in preparation of the DSC over DP support. This > does not handle non-hotpluggable displays. Good, because my question is specifically about *hotpluggable* displays/panels like the upcoming DSC support for DP. After all there would be no point in me suggesting to connect and disconnect non-hotpluggable displays and expect something sensible to happen, wouldn't it? Allow me to copy-paste the question again for convenience, with some minor wording changes: However, if I connect a DSC DP display, then disconnect it (now the register should be non-zero, but cfg->dsc will be zero), and then connect and reconnect a non-DSC DP display multiple times, it'll get flushed every time because we never clear CTL_DSC_FLUSH after that? And the missing part is: would multiple flushes be harmful in this case? > I do not think dynamic switch > between DSC and non-DSC of non-hotpluggable displays needs to be > discussed here as its not handled at all with or without this patch. > > We wanted to get early reviews on the patch. If you want this patch to > be absorbed when rest of DSC over DP lands, I have no concerns with > that. I wont pick this up for fixes and we will land this together with > the rest of DP over DSC. I don't mind when and where this lands, just want to have the semantics clear around persisting the value of CTL_DSC_FLUSh in the register. Regardless, this patch doesn't sound like a fix but a workaround until reset_intf_cfg() is fixed to be called at the right point, and extended to clear CTL_DSC_ACTIVE and flush the DSCs. Perhaps it shouldn't have a Fixes: tag for that reason, as you intend to reinstate this if (cfg->dsc) condition when that is done? - Marijn
On 4/14/2023 4:11 PM, Marijn Suijten wrote: > On 2023-04-14 10:57:45, Abhinav Kumar wrote: >> On 4/14/2023 10:34 AM, Marijn Suijten wrote: >>> On 2023-04-14 08:48:43, Abhinav Kumar wrote: >>>> On 4/14/2023 12:35 AM, Marijn Suijten wrote: >>>>> On 2023-04-12 10:33:15, Abhinav Kumar wrote: >>>>> [..] >>>>>>> What happens if a device boots without DSC panel connected? Will >>>>>>> CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the >>>>>>> DSC blocks? Or could this flush uninitialized state to the block? >>>>>> >>>>>> If we bootup without DSC panel connected, the kernel's cfg->dsc will be >>>>>> 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush >>>>>> any DSC blocks. >>>>> >>>>> Ack, that makes sense. However, if I connect a DSC panel, then >>>>> disconnect it (now the register should be non-zero, but cfg->dsc will be >>>>> zero), and then replug a non-DSC panel multiple times, it'll get flushed >>>>> every time because we never clear CTL_DSC_FLUSH after that? >>>> >>>> If we remove it after kernel starts, that issue is there even today >>>> without that change because DSI is not a hot-pluggable display so a >>>> teardown wont happen when you plug out the panel. How will cfg->dsc be 0 >>>> then? In that case, its not a valid test as there was no indication to >>>> DRM that display was disconnected so we cannot tear it down. >>> >>> The patch description itself describes hot-pluggable displays, which I >>> believe is the upcoming DSC support for DP? You ask how cfg->dsc can >>> become zero, but this is **exactly** what the patch description >>> describes, and what this patch is removing the `if` for. If we are not >>> allowed to discuss that scenario because it is not currently supported, >>> neither should we allow to apply this patch. >>> >>> With that in mind, can you re-answer the question? >> >> I didnt follow what needs to be re-answered. >> >> This patch is being sent in preparation of the DSC over DP support. This >> does not handle non-hotpluggable displays. > > Good, because my question is specifically about *hotpluggable* > displays/panels like the upcoming DSC support for DP. After all there > would be no point in me suggesting to connect and disconnect > non-hotpluggable displays and expect something sensible to happen, > wouldn't it? Allow me to copy-paste the question again for convenience, > with some minor wording changes: > > However, if I connect a DSC DP display, then disconnect it (now the > register should be non-zero, but cfg->dsc will be zero), and then > connect and reconnect a non-DSC DP display multiple times, it'll get > flushed every time because we never clear CTL_DSC_FLUSH after that? > > And the missing part is: would multiple flushes be harmful in this case? Well, you kept asking about "DSC panel" , that made me think you were asking about a non-hotpluggable MIPI DSI DSC panel and not DP DSC monitor. On many boards, panels can be removed/connected back to their daughter card. The term "panel" confused me a bit. Now answering your question. Yes, it will get flushed once every hotplug thats not too bad but importantly DSC wont be active as CTL_DSC_ACTIVE will be set to 0 so it wont cause any issue. >> I do not think dynamic switch >> between DSC and non-DSC of non-hotpluggable displays needs to be >> discussed here as its not handled at all with or without this patch. >> >> We wanted to get early reviews on the patch. If you want this patch to >> be absorbed when rest of DSC over DP lands, I have no concerns with >> that. I wont pick this up for fixes and we will land this together with >> the rest of DP over DSC. > > I don't mind when and where this lands, just want to have the semantics > clear around persisting the value of CTL_DSC_FLUSh in the register. > > Regardless, this patch doesn't sound like a fix but a workaround until > reset_intf_cfg() is fixed to be called at the right point, and extended > to clear CTL_DSC_ACTIVE and flush the DSCs. Perhaps it shouldn't have a > Fixes: tag for that reason, as you intend to reinstate this > if (cfg->dsc) condition when that is done? > Its certainly fixing the use-case of DSC to non-DSC switching. So it is a fix. But yes not the best fix possible. We have to improve it by moving this to reset_intf_cfg() as I already committed to. > - Marijn
On 2023-04-14 16:51:52, Abhinav Kumar wrote: > On 4/14/2023 4:11 PM, Marijn Suijten wrote: > > On 2023-04-14 10:57:45, Abhinav Kumar wrote: > >> On 4/14/2023 10:34 AM, Marijn Suijten wrote: > >>> On 2023-04-14 08:48:43, Abhinav Kumar wrote: > >>>> On 4/14/2023 12:35 AM, Marijn Suijten wrote: > >>>>> On 2023-04-12 10:33:15, Abhinav Kumar wrote: > >>>>> [..] > >>>>>>> What happens if a device boots without DSC panel connected? Will > >>>>>>> CTL_DSC_FLUSH be zero and not (unnecessarily, I assume) flush any of the > >>>>>>> DSC blocks? Or could this flush uninitialized state to the block? > >>>>>> > >>>>>> If we bootup without DSC panel connected, the kernel's cfg->dsc will be > >>>>>> 0 and default register value of CTL_DSC_FLUSH will be 0 so it wont flush > >>>>>> any DSC blocks. > >>>>> > >>>>> Ack, that makes sense. However, if I connect a DSC panel, then > >>>>> disconnect it (now the register should be non-zero, but cfg->dsc will be > >>>>> zero), and then replug a non-DSC panel multiple times, it'll get flushed > >>>>> every time because we never clear CTL_DSC_FLUSH after that? > >>>> > >>>> If we remove it after kernel starts, that issue is there even today > >>>> without that change because DSI is not a hot-pluggable display so a > >>>> teardown wont happen when you plug out the panel. How will cfg->dsc be 0 > >>>> then? In that case, its not a valid test as there was no indication to > >>>> DRM that display was disconnected so we cannot tear it down. > >>> > >>> The patch description itself describes hot-pluggable displays, which I > >>> believe is the upcoming DSC support for DP? You ask how cfg->dsc can > >>> become zero, but this is **exactly** what the patch description > >>> describes, and what this patch is removing the `if` for. If we are not > >>> allowed to discuss that scenario because it is not currently supported, > >>> neither should we allow to apply this patch. > >>> > >>> With that in mind, can you re-answer the question? > >> > >> I didnt follow what needs to be re-answered. > >> > >> This patch is being sent in preparation of the DSC over DP support. This > >> does not handle non-hotpluggable displays. > > > > Good, because my question is specifically about *hotpluggable* > > displays/panels like the upcoming DSC support for DP. After all there > > would be no point in me suggesting to connect and disconnect > > non-hotpluggable displays and expect something sensible to happen, > > wouldn't it? Allow me to copy-paste the question again for convenience, > > with some minor wording changes: > > > > However, if I connect a DSC DP display, then disconnect it (now the > > register should be non-zero, but cfg->dsc will be zero), and then > > connect and reconnect a non-DSC DP display multiple times, it'll get > > flushed every time because we never clear CTL_DSC_FLUSH after that? > > > > And the missing part is: would multiple flushes be harmful in this case? > > Well, you kept asking about "DSC panel" , that made me think you were > asking about a non-hotpluggable MIPI DSI DSC panel and not DP DSC > monitor. On many boards, panels can be removed/connected back to their > daughter card. The term "panel" confused me a bit. > > Now answering your question. > > Yes, it will get flushed once every hotplug thats not too bad but > importantly DSC wont be active as CTL_DSC_ACTIVE will be set to 0 so it > wont cause any issue. > > > >> I do not think dynamic switch > >> between DSC and non-DSC of non-hotpluggable displays needs to be > >> discussed here as its not handled at all with or without this patch. > >> > >> We wanted to get early reviews on the patch. If you want this patch to > >> be absorbed when rest of DSC over DP lands, I have no concerns with > >> that. I wont pick this up for fixes and we will land this together with > >> the rest of DP over DSC. > > > > I don't mind when and where this lands, just want to have the semantics > > clear around persisting the value of CTL_DSC_FLUSh in the register. > > > > Regardless, this patch doesn't sound like a fix but a workaround until > > reset_intf_cfg() is fixed to be called at the right point, and extended > > to clear CTL_DSC_ACTIVE and flush the DSCs. Perhaps it shouldn't have a > > Fixes: tag for that reason, as you intend to reinstate this > > if (cfg->dsc) condition when that is done? > > > > Its certainly fixing the use-case of DSC to non-DSC switching. So it is > a fix. > > But yes not the best fix possible. We have to improve it by moving this > to reset_intf_cfg() as I already committed to. Ack, thanks for confirming this all and working on that, sounds good! - Marijn
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c index bbdc95c..88e4efe 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c @@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx, if (cfg->merge_3d) DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE, BIT(cfg->merge_3d - MERGE_3D_0)); - if (cfg->dsc) { - DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); - DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); - } + + DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX); + DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc); } static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
In current code, the dsc active bits are set only if the cfg->dsc is set. However, for displays which are hot-pluggable, there can be a use-case of disconnecting a DSC supported sink and connecting a non-DSC sink. For those cases we need to clear DSC active bits during teardown. Fixes: ede3c6bb00c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl") Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)