| Message ID | 20240417-drm-msm-initial-dualpipe-dsc-fixes-v1-5-78ae3ee9a697@somainline.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: Initial fixes for DUALPIPE (+DSC) topology | expand |
On Wed, Apr 17, 2024 at 01:57:45AM +0200, Marijn Suijten wrote: > This comment one line down references a single, "same CTL" that controls > two interfaces, so the comment should clearly describe two interfaces > used with a single active CTL and not "two CTLs". > > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > index d9e7dbf0499c..7e849fe74801 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 > @@ -428,7 +428,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) > dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > > /* > - * For single flush cases (dual-ctl or pp-split), skip setting the > + * For single flush cases (dual-intf or pp-split), skip setting the It should be fixed, but in the other way: it's 'single-ctl'. See sde_encoder_phys_needs_single_flush(). > * flush bit for the slave intf, since both intfs use same ctl > * and HW will only flush the master. > */ > > -- > 2.44.0 >
On 2024-04-18 02:30:59, Dmitry Baryshkov wrote: > On Wed, Apr 17, 2024 at 01:57:45AM +0200, Marijn Suijten wrote: > > This comment one line down references a single, "same CTL" that controls > > two interfaces, so the comment should clearly describe two interfaces > > used with a single active CTL and not "two CTLs". > > > > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > index d9e7dbf0499c..7e849fe74801 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 > > @@ -428,7 +428,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) > > dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > > > > /* > > - * For single flush cases (dual-ctl or pp-split), skip setting the > > + * For single flush cases (dual-intf or pp-split), skip setting the > > It should be fixed, but in the other way: it's 'single-ctl'. See > sde_encoder_phys_needs_single_flush(). As written in the cover letter I was unsure about this comment. You are right that sde_encoder_phys_needs_single_flush() is supposed to return true in pp-split or single-ctl. However, the second part of the comment (right below) is in conflict with another patch that I've sent as part of these series: on a single-ctl setup with dual interfaces, the second INTF needs to be marked for flushing. While that observation and fix is for CMD-mode, the exact same comment is found downstream for video mode: https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.4.r1-rel/msm/sde/sde_encoder_phys_vid.c?ref_type=heads#L794-804 You were fixing exactly that in one of your preliminary Active-CTL patches by making dpu_encoder_phys_vid_needs_single_flush() return for Active-CTL, so we should probably update this comment in the same patch when you send it? (that is: the flush bit needs to be set for the slave intf in Active-CTL. Before Active-CTL, a slave encoder would actually have two CTLs and two INTFs where the flush bit was probably skipped on both slaves?) On a side-note, since the needs_single_flush callback is used elsehwere, I'm unsure if that patch affects things in the wrong way since the downstream file linked above applies the check for CTL_ACTIVE_CFG directly in-line without affecting the callback. - Marijn > > * flush bit for the slave intf, since both intfs use same ctl > > * and HW will only flush the master. > > */ > > > > -- > > 2.44.0 > > > > -- > With best wishes > Dmitry
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 d9e7dbf0499c..7e849fe74801 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 @@ -428,7 +428,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) dpu_encoder_phys_vid_setup_timing_engine(phys_enc); /* - * For single flush cases (dual-ctl or pp-split), skip setting the + * For single flush cases (dual-intf or pp-split), skip setting the * flush bit for the slave intf, since both intfs use same ctl * and HW will only flush the master. */
This comment one line down references a single, "same CTL" that controls two interfaces, so the comment should clearly describe two interfaces used with a single active CTL and not "two CTLs". Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)