| Message ID | 20240603-dpu-mode-config-width-v2-7-16af520575a6@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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 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) { > > > > > > > > > >
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) { 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) {
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(-)