| Message ID | 20240603-dpu-mode-config-width-v2-0-16af520575a6@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dpu: be more friendly to X.org | expand |
On 6/2/2024 2:39 PM, Dmitry Baryshkov wrote: > Check in _dpu_crtc_setup_lm_bounds() that CRTC width is not overflowing > LM requirements. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index e3b1e5f55a92..c5e874a3656a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -711,12 +711,13 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc) > _dpu_crtc_complete_flip(crtc); > } > > -static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > +static int _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > struct drm_crtc_state *state) Perhaps now we need to rename this to _dpu_crtc_setup_and_check_lm_bounds()? Also, prior to this change, we never had a bounds check for each LM which we should have had . Does this qualify for a Fixes tag? With those two questions addressed, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > { > struct dpu_crtc_state *cstate = to_dpu_crtc_state(state); > struct drm_display_mode *adj_mode = &state->adjusted_mode; > u32 crtc_split_width = adj_mode->hdisplay / cstate->num_mixers; > + struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > int i; > > for (i = 0; i < cstate->num_mixers; i++) { > @@ -727,7 +728,12 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > r->y2 = adj_mode->vdisplay; > > trace_dpu_crtc_setup_lm_bounds(DRMID(crtc), i, r); > + > + if (drm_rect_width(r) > dpu_kms->catalog->caps->max_mixer_width) > + return -E2BIG; > } > + > + return 0; > } > > static void _dpu_crtc_get_pcc_coeff(struct drm_crtc_state *state, > @@ -1195,8 +1201,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > if (crtc_state->active_changed) > crtc_state->mode_changed = true; > > - if (cstate->num_mixers) > - _dpu_crtc_setup_lm_bounds(crtc, crtc_state); > + if (cstate->num_mixers) { > + rc = _dpu_crtc_setup_lm_bounds(crtc, crtc_state); > + if (rc) > + return rc; > + } > > /* FIXME: move this to dpu_plane_atomic_check? */ > drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { >
On Mon, 3 Jun 2024 at 23:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 6/2/2024 2:39 PM, Dmitry Baryshkov wrote: > > Check in _dpu_crtc_setup_lm_bounds() that CRTC width is not overflowing > > LM requirements. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index e3b1e5f55a92..c5e874a3656a 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -711,12 +711,13 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc) > > _dpu_crtc_complete_flip(crtc); > > } > > > > -static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > > +static int _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > > struct drm_crtc_state *state) > > Perhaps now we need to rename this to _dpu_crtc_setup_and_check_lm_bounds()? Ack, I'll rename it. > > Also, prior to this change, we never had a bounds check for each LM > which we should have had . Does this qualify for a Fixes tag? Probably no. We currently have a limit in the drm_mode_config, which ensures that the CRTC isn't too wide. > > With those two questions addressed, > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > { > > struct dpu_crtc_state *cstate = to_dpu_crtc_state(state); > > struct drm_display_mode *adj_mode = &state->adjusted_mode; > > u32 crtc_split_width = adj_mode->hdisplay / cstate->num_mixers; > > + struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > > int i; > > > > for (i = 0; i < cstate->num_mixers; i++) { > > @@ -727,7 +728,12 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > > r->y2 = adj_mode->vdisplay; > > > > trace_dpu_crtc_setup_lm_bounds(DRMID(crtc), i, r); > > + > > + if (drm_rect_width(r) > dpu_kms->catalog->caps->max_mixer_width) > > + return -E2BIG; > > } > > > + > > + return 0; > > } > > > > static void _dpu_crtc_get_pcc_coeff(struct drm_crtc_state *state, > > @@ -1195,8 +1201,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > > if (crtc_state->active_changed) > > crtc_state->mode_changed = true; > > > > - if (cstate->num_mixers) > > - _dpu_crtc_setup_lm_bounds(crtc, crtc_state); > > + if (cstate->num_mixers) { > > + rc = _dpu_crtc_setup_lm_bounds(crtc, crtc_state); > > + if (rc) > > + return rc; > > + } > > > > /* FIXME: move this to dpu_plane_atomic_check? */ > > drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { > >
On 6/3/2024 2:16 PM, Dmitry Baryshkov wrote: > On Mon, 3 Jun 2024 at 23:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 6/2/2024 2:39 PM, Dmitry Baryshkov wrote: >>> Check in _dpu_crtc_setup_lm_bounds() that CRTC width is not overflowing >>> LM requirements. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++++++++++--- >>> 1 file changed, 12 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> index e3b1e5f55a92..c5e874a3656a 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> @@ -711,12 +711,13 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc) >>> _dpu_crtc_complete_flip(crtc); >>> } >>> >>> -static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, >>> +static int _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, >>> struct drm_crtc_state *state) >> >> Perhaps now we need to rename this to _dpu_crtc_setup_and_check_lm_bounds()? > > Ack, I'll rename it. > >> >> Also, prior to this change, we never had a bounds check for each LM >> which we should have had . Does this qualify for a Fixes tag? > > Probably no. We currently have a limit in the drm_mode_config, which > ensures that the CRTC isn't too wide. > The limit in drm_mode_config is to ensure we will not go beyond 2*max_mixer_width for the mode as we support only upto 2 LMs. This check is making sure that even for the single LM, we do not go beyond the max_mixer_width which is valid imo. >> >> With those two questions addressed, >> >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >>> { >>> struct dpu_crtc_state *cstate = to_dpu_crtc_state(state); >>> struct drm_display_mode *adj_mode = &state->adjusted_mode; >>> u32 crtc_split_width = adj_mode->hdisplay / cstate->num_mixers; >>> + struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); >>> int i; >>> >>> for (i = 0; i < cstate->num_mixers; i++) { >>> @@ -727,7 +728,12 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, >>> r->y2 = adj_mode->vdisplay; >>> >>> trace_dpu_crtc_setup_lm_bounds(DRMID(crtc), i, r); >>> + >>> + if (drm_rect_width(r) > dpu_kms->catalog->caps->max_mixer_width) >>> + return -E2BIG; >>> } >> >>> + >>> + return 0; >>> } >>> >>> static void _dpu_crtc_get_pcc_coeff(struct drm_crtc_state *state, >>> @@ -1195,8 +1201,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, >>> if (crtc_state->active_changed) >>> crtc_state->mode_changed = true; >>> >>> - if (cstate->num_mixers) >>> - _dpu_crtc_setup_lm_bounds(crtc, crtc_state); >>> + if (cstate->num_mixers) { >>> + rc = _dpu_crtc_setup_lm_bounds(crtc, crtc_state); >>> + if (rc) >>> + return rc; >>> + } >>> >>> /* FIXME: move this to dpu_plane_atomic_check? */ >>> drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { >>> > > >
On 6/2/2024 2:39 PM, Dmitry Baryshkov wrote: > Lift mode_config limits set by the DPU driver to the actual FB limits as > handled by the dpu_plane.c. Move 2*max_lm_width check where it belongs, > to the drm_crtc_helper_funcs::mode_valid() callback. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++++++++++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++------- > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index c5e874a3656a..8cf063e4c09d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1236,6 +1236,20 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > return 0; > } > With the added checks in dpu_crtc_mode_valid(), I am fine with this as it will retain the previous limits in drm_mode_config. Hence, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On Mon, Jun 03, 2024 at 02:24:27PM -0700, Abhinav Kumar wrote: > > > On 6/3/2024 2:16 PM, Dmitry Baryshkov wrote: > > On Mon, 3 Jun 2024 at 23:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > > > > > On 6/2/2024 2:39 PM, Dmitry Baryshkov wrote: > > > > Check in _dpu_crtc_setup_lm_bounds() that CRTC width is not overflowing > > > > LM requirements. > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++++++++++--- > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > index e3b1e5f55a92..c5e874a3656a 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > @@ -711,12 +711,13 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc) > > > > _dpu_crtc_complete_flip(crtc); > > > > } > > > > > > > > -static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > > > > +static int _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > > > > struct drm_crtc_state *state) > > > > > > Perhaps now we need to rename this to _dpu_crtc_setup_and_check_lm_bounds()? > > > > Ack, I'll rename it. > > > > > > > > Also, prior to this change, we never had a bounds check for each LM > > > which we should have had . Does this qualify for a Fixes tag? > > > > Probably no. We currently have a limit in the drm_mode_config, which > > ensures that the CRTC isn't too wide. > > > > The limit in drm_mode_config is to ensure we will not go beyond > 2*max_mixer_width for the mode as we support only upto 2 LMs. > > This check is making sure that even for the single LM, we do not go beyond > the max_mixer_width which is valid imo. For the case where 2*LM mode is not possible? Yes, it seems to be a valid case. But in such case I'd prefer to rework this patch and pull it to the top of the series. I'll take a look. > > > > > > > With those two questions addressed, > > > > > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > > > > > { > > > > struct dpu_crtc_state *cstate = to_dpu_crtc_state(state); > > > > struct drm_display_mode *adj_mode = &state->adjusted_mode; > > > > u32 crtc_split_width = adj_mode->hdisplay / cstate->num_mixers; > > > > + struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > > > > int i; > > > > > > > > for (i = 0; i < cstate->num_mixers; i++) { > > > > @@ -727,7 +728,12 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > > > > r->y2 = adj_mode->vdisplay; > > > > > > > > trace_dpu_crtc_setup_lm_bounds(DRMID(crtc), i, r); > > > > + > > > > + if (drm_rect_width(r) > dpu_kms->catalog->caps->max_mixer_width) > > > > + return -E2BIG; > > > > } > > > > > > > + > > > > + return 0; > > > > } > > > > > > > > static void _dpu_crtc_get_pcc_coeff(struct drm_crtc_state *state, > > > > @@ -1195,8 +1201,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > > > > if (crtc_state->active_changed) > > > > crtc_state->mode_changed = true; > > > > > > > > - if (cstate->num_mixers) > > > > - _dpu_crtc_setup_lm_bounds(crtc, crtc_state); > > > > + if (cstate->num_mixers) { > > > > + rc = _dpu_crtc_setup_lm_bounds(crtc, crtc_state); > > > > + if (rc) > > > > + return rc; > > > > + } > > > > > > > > /* FIXME: move this to dpu_plane_atomic_check? */ > > > > drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { > > > > > > > > > >
Unlike other compositors X.org allocates a single framebuffer covering the whole screen space. This is not an issue with the single screens, but with the multi-monitor setup 5120x4096 becomes a limiting factor. Check the hardware-bound limitations and lift the FB max size to 16383x16383. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Changes in v2: - Added dpu_crtc_valid() to verify that 2*lm_width limit is enforced (Abhinav) - Link to v1: https://lore.kernel.org/r/20240319-dpu-mode-config-width-v1-0-d0fe6bf81bf1@linaro.org --- Dmitry Baryshkov (9): drm/msm/dpu: drop dpu_format_check_modified_format drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update drm/msm/dpu: split dpu_format_populate_layout drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check drm/msm/dpu: check for the plane pitch overflow drm/msm/dpu: drop call to _dpu_crtc_setup_lm_bounds from atomic_begin drm/msm/dpu: check for overflow in _dpu_crtc_setup_lm_bounds() drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 32 ++++++-- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 8 +- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 91 ++++++---------------- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 24 ++---- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 37 +++++---- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 3 + drivers/gpu/drm/msm/msm_kms.h | 6 -- 10 files changed, 91 insertions(+), 126 deletions(-) --- base-commit: 0e1980c40b6edfa68b6acf926bab22448a6e40c9 change-id: 20240318-dpu-mode-config-width-626d3c7ad52a Best regards,