| Message ID | 1683061382-32651-5-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add DSC 1.2 dpu supports | expand |
On 03/05/2023 00:02, Kuogee Hsieh wrote: > During DSC setup, the crossbar mux need to be programmed to engage > DSC to specified PINGPONG. Hence during tear down, the crossbar mux > need to be reset to disengage DSC from PINGPONG. This patch add > PINGPONG_NONE to serve as disable to reset crossbar mux. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 7 +++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 1 - > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++- > 4 files changed, 6 insertions(+), 7 deletions(-)
On 2023-05-02 14:02:59, Kuogee Hsieh wrote: > During DSC setup, the crossbar mux need to be programmed to engage > DSC to specified PINGPONG. Hence during tear down, the crossbar mux > need to be reset to disengage DSC from PINGPONG. This patch add > PINGPONG_NONE to serve as disable to reset crossbar mux. This patch doesn't *just add* PINGPONG_NONE to reset the crossbar; that functionality was already available thanks to a `bool enable` function parameter. Instead it should explain why you think PINGPONG_NONE is more convenient than passing a bool that warrants this replacement. (Hint: I think because you don't have a hw_pp->idx available in the teardown path, and/or its value is not relevant for the disable case anyway.) In addition I don't see this series use PINGPONG_NONE anywhere yet: will that be added in the DSC 1.2 series for DP (to support hotplug)? > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 7 +++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 1 - > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++- > 4 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 1dc5dbe..d9ad334 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1839,7 +1839,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, > hw_pp->ops.setup_dsc(hw_pp); > > if (hw_dsc->ops.dsc_bind_pingpong_blk) > - hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx); > + hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, hw_pp->idx); > > if (hw_pp->ops.enable_dsc) > hw_pp->ops.enable_dsc(hw_pp); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c > index 4a6bbcc..3e68d47 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c > @@ -157,7 +157,6 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc, > > static void dpu_hw_dsc_bind_pingpong_blk( > struct dpu_hw_dsc *hw_dsc, > - bool enable, > const enum dpu_pingpong pp) > { > struct dpu_hw_blk_reg_map *c = &hw_dsc->hw; > @@ -166,13 +165,13 @@ static void dpu_hw_dsc_bind_pingpong_blk( > > dsc_ctl_offset = DSC_CTL(hw_dsc->idx); > > - if (enable) > + if (pp) > mux_cfg = (pp - PINGPONG_0) & 0x7; > > DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n", > - enable ? "Binding" : "Unbinding", > + pp ? "Binding" : "Unbinding", > hw_dsc->idx - DSC_0, > - enable ? "to" : "from", > + pp ? "to" : "from", > pp - PINGPONG_0); PINGPONG_NONE - PINGPONG_0 = -1, so this whole debug log likely needs to be rewritten for the disable case as we don't know what PINGPONG it is being unbound from. How about: if (pp) DRM_DEBUG_KMS("Binding dsc:%d to pp:%d\n", hw_dsc->idx - DSC_0, pp - PINGPONG_0); else DRM_DEBUG_KMS("Unbinding dsc:%d from any pp\n", hw_dsc->idx - DSC_0); - Marijn > > DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h > index 287ec5f..138080a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h > @@ -44,7 +44,6 @@ struct dpu_hw_dsc_ops { > struct drm_dsc_config *dsc); > > void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc, > - bool enable, > enum dpu_pingpong pp); > }; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h > index 2d9192a..56826a9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h > @@ -191,7 +191,8 @@ enum dpu_dsc { > }; > > enum dpu_pingpong { > - PINGPONG_0 = 1, > + PINGPONG_NONE, > + PINGPONG_0, > PINGPONG_1, > PINGPONG_2, > PINGPONG_3, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 5/3/2023 1:03 AM, Marijn Suijten wrote: > On 2023-05-02 14:02:59, Kuogee Hsieh wrote: >> During DSC setup, the crossbar mux need to be programmed to engage >> DSC to specified PINGPONG. Hence during tear down, the crossbar mux >> need to be reset to disengage DSC from PINGPONG. This patch add >> PINGPONG_NONE to serve as disable to reset crossbar mux. > This patch doesn't *just add* PINGPONG_NONE to reset the crossbar; that > functionality was already available thanks to a `bool enable` function > parameter. Instead it should explain why you think PINGPONG_NONE is > more convenient than passing a bool that warrants this replacement. > (Hint: I think because you don't have a hw_pp->idx available in the > teardown path, and/or its value is not relevant for the disable case > anyway.) > > In addition I don't see this series use PINGPONG_NONE anywhere yet: will > that be added in the DSC 1.2 series for DP (to support hotplug)? PINGPONG_NONE will be used to tear down DSC data path later at DP DSC patch series. Current it is not used because DSI does not do tear down. > >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 7 +++---- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 1 - >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++- >> 4 files changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 1dc5dbe..d9ad334 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -1839,7 +1839,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, >> hw_pp->ops.setup_dsc(hw_pp); >> >> if (hw_dsc->ops.dsc_bind_pingpong_blk) >> - hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx); >> + hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, hw_pp->idx); >> >> if (hw_pp->ops.enable_dsc) >> hw_pp->ops.enable_dsc(hw_pp); >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >> index 4a6bbcc..3e68d47 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c >> @@ -157,7 +157,6 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc, >> >> static void dpu_hw_dsc_bind_pingpong_blk( >> struct dpu_hw_dsc *hw_dsc, >> - bool enable, >> const enum dpu_pingpong pp) >> { >> struct dpu_hw_blk_reg_map *c = &hw_dsc->hw; >> @@ -166,13 +165,13 @@ static void dpu_hw_dsc_bind_pingpong_blk( >> >> dsc_ctl_offset = DSC_CTL(hw_dsc->idx); >> >> - if (enable) >> + if (pp) >> mux_cfg = (pp - PINGPONG_0) & 0x7; >> >> DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n", >> - enable ? "Binding" : "Unbinding", >> + pp ? "Binding" : "Unbinding", >> hw_dsc->idx - DSC_0, >> - enable ? "to" : "from", >> + pp ? "to" : "from", >> pp - PINGPONG_0); > PINGPONG_NONE - PINGPONG_0 = -1, so this whole debug log likely needs to > be rewritten for the disable case as we don't know what PINGPONG it is > being unbound from. How about: > > if (pp) > DRM_DEBUG_KMS("Binding dsc:%d to pp:%d\n", > hw_dsc->idx - DSC_0, > pp - PINGPONG_0); > else > DRM_DEBUG_KMS("Unbinding dsc:%d from any pp\n", > hw_dsc->idx - DSC_0); > > - Marijn > >> >> DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg); >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h >> index 287ec5f..138080a 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h >> @@ -44,7 +44,6 @@ struct dpu_hw_dsc_ops { >> struct drm_dsc_config *dsc); >> >> void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc, >> - bool enable, >> enum dpu_pingpong pp); >> }; >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h >> index 2d9192a..56826a9 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h >> @@ -191,7 +191,8 @@ enum dpu_dsc { >> }; >> >> enum dpu_pingpong { >> - PINGPONG_0 = 1, >> + PINGPONG_NONE, >> + PINGPONG_0, >> PINGPONG_1, >> PINGPONG_2, >> PINGPONG_3, >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 1dc5dbe..d9ad334 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1839,7 +1839,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, hw_pp->ops.setup_dsc(hw_pp); if (hw_dsc->ops.dsc_bind_pingpong_blk) - hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx); + hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, hw_pp->idx); if (hw_pp->ops.enable_dsc) hw_pp->ops.enable_dsc(hw_pp); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c index 4a6bbcc..3e68d47 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c @@ -157,7 +157,6 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc, static void dpu_hw_dsc_bind_pingpong_blk( struct dpu_hw_dsc *hw_dsc, - bool enable, const enum dpu_pingpong pp) { struct dpu_hw_blk_reg_map *c = &hw_dsc->hw; @@ -166,13 +165,13 @@ static void dpu_hw_dsc_bind_pingpong_blk( dsc_ctl_offset = DSC_CTL(hw_dsc->idx); - if (enable) + if (pp) mux_cfg = (pp - PINGPONG_0) & 0x7; DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n", - enable ? "Binding" : "Unbinding", + pp ? "Binding" : "Unbinding", hw_dsc->idx - DSC_0, - enable ? "to" : "from", + pp ? "to" : "from", pp - PINGPONG_0); DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h index 287ec5f..138080a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h @@ -44,7 +44,6 @@ struct dpu_hw_dsc_ops { struct drm_dsc_config *dsc); void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc, - bool enable, enum dpu_pingpong pp); }; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h index 2d9192a..56826a9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h @@ -191,7 +191,8 @@ enum dpu_dsc { }; enum dpu_pingpong { - PINGPONG_0 = 1, + PINGPONG_NONE, + PINGPONG_0, PINGPONG_1, PINGPONG_2, PINGPONG_3,
During DSC setup, the crossbar mux need to be programmed to engage DSC to specified PINGPONG. Hence during tear down, the crossbar mux need to be reset to disengage DSC from PINGPONG. This patch add PINGPONG_NONE to serve as disable to reset crossbar mux. Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 7 +++---- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++- 4 files changed, 6 insertions(+), 7 deletions(-)