Re: [PATCH] drm/i915/dp: Fix connector DSC HW state readout
On Mon, Feb 05, 2024 at 03:26:31PM +0200, Imre Deak wrote:
> The DSC HW state of DP connectors is read out during driver loading and
> system resume in intel_modeset_update_connector_atomic_state(). This
> function is called for all connectors though and so the state of DSI
> connectors will also get updated incorrectly, triggering a WARN there
> wrt. the DSC decompression AUX device.
>
> Fix the above by moving the DSC state readout to a new DP connector
> specific sync_state() hook. This is anyway the logical place to update
> the connector object's state vs. the connector's atomic state.
>
> Fixes: b2608c6b3212 ("drm/i915/dp_mst: Enable MST DSC decompression for all
> streams")
> Reported-by: Drew Davenport
> Closes: https://lore.kernel.org/all/zb0q8idvxs0hx...@chromium.org
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/display/intel_display_types.h | 7 +++
> drivers/gpu/drm/i915/display/intel_dp.c| 13 +
> drivers/gpu/drm/i915/display/intel_dp.h| 2 ++
> drivers/gpu/drm/i915/display/intel_dp_mst.c| 1 +
> drivers/gpu/drm/i915/display/intel_modeset_setup.c | 13 ++---
> 5 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index ae2e8cff9d691..6e1ddd05bd504 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -609,6 +609,13 @@ struct intel_connector {
>* and active (i.e. dpms ON state). */
> bool (*get_hw_state)(struct intel_connector *);
>
> + /*
> + * Optional hook called during init/resume to sync any state
> + * stored in the connector (eg. DSC state) wrt. the HW state.
> + */
> + void (*sync_state)(struct intel_connector *connector,
> +const struct intel_crtc_state *crtc_state);
> +
> /* Panel info for eDP and LVDS */
> struct intel_panel panel;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index ab415f41924d7..f8b1aab7745fd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5868,6 +5868,19 @@ intel_dp_connector_unregister(struct drm_connector
> *connector)
> intel_connector_unregister(connector);
> }
>
> +void intel_dp_connector_sync_state(struct intel_connector *connector,
> +const struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> +
> + if (crtc_state && crtc_state->dsc.compression_enable) {
> + drm_WARN_ON(&i915->drm, !connector->dp.dsc_decompression_aux);
> + connector->dp.dsc_decompression_enabled = true;
> + } else {
> + connector->dp.dsc_decompression_enabled = false;
> + }
> +}
> +
> void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
> {
> struct intel_digital_port *dig_port =
> enc_to_dig_port(to_intel_encoder(encoder));
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> b/drivers/gpu/drm/i915/display/intel_dp.h
> index 530cc97bc42f4..f911dfab5fba9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -45,6 +45,8 @@ bool intel_dp_limited_color_range(const struct
> intel_crtc_state *crtc_state,
> int intel_dp_min_bpp(enum intel_output_format output_format);
> bool intel_dp_init_connector(struct intel_digital_port *dig_port,
>struct intel_connector *intel_connector);
> +void intel_dp_connector_sync_state(struct intel_connector *connector,
> +const struct intel_crtc_state *crtc_state);
> void intel_dp_set_link_params(struct intel_dp *intel_dp,
> int link_rate, int lane_count);
> int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 5fa25a5a36b55..130c6aab86b22 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1538,6 +1538,7 @@ static struct drm_connector
> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> return NULL;
>
> intel_connector->get_hw_state = intel_dp_mst_get_hw_state;
> + intel_connector->sync_state = intel_dp_connector_sync_state;
> intel_connector->mst_port = intel_dp;
> intel_connector->port = port;
>
Re: [v5, 25/30] drm/i915/dp_mst: Enable MST DSC decompression for all streams
On Tue, Nov 07, 2023 at 02:15:03AM +0200, Imre Deak wrote:
> Enable DSC decompression for all streams. In particular atm if a sink is
> connected to a last branch device that is downstream of the first branch
> device connected to the source, decompression is not enabled for it.
> Similarly it's not enabled if the sink supports this with the last
> branch device passing through the compressed stream to it.
>
> Enable DSC in the above cases as well. Since last branch devices may
> handle the decompression for multiple ports, toggling DSC needs to be
> refcounted, add this using the DSC AUX device as a reference.
>
> v2:
> - Fix refcounting, setting/clearing
> connector->dp.dsc_decompression_enabled always as needed. (Stan)
> - Make the refcounting more uniform for the SST vs. MST case.
> - Add state checks for connector->dp.dsc_decompression_enabled and
> connector crtc.
> - Sanitize connector DSC decompression state during HW setup.
> - s/use_count/ref_count/
> v3:
> - Remove stale TODO: comment to set the actual decompression_aux.
>
> Cc: Stanislav Lisovskiy
> Signed-off-by: Imre Deak
> Reviewed-by: Stanislav Lisovskiy
> ---
> .../drm/i915/display/intel_display_types.h| 1 +
> drivers/gpu/drm/i915/display/intel_dp.c | 72 ++-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 24 ++-
> .../drm/i915/display/intel_modeset_setup.c| 6 ++
> 4 files changed, 82 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 6c2f18ef543e4..0a5508c90e8bc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -626,6 +626,7 @@ struct intel_connector {
> u8 fec_capability;
>
> u8 dsc_hblank_expansion_quirk:1;
> + u8 dsc_decompression_enabled:1;
> } dp;
>
> /* Work struct to schedule a uevent on link train failure */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index bea0c03b94835..3fee371529f17 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1403,6 +1403,7 @@ static bool intel_dp_supports_dsc(const struct
> intel_connector *connector,
> return false;
>
> return intel_dsc_source_support(crtc_state) &&
> + connector->dp.dsc_decompression_aux &&
> drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd);
> }
>
> @@ -2986,6 +2987,65 @@ intel_dp_sink_set_dsc_passthrough(const struct
> intel_connector *connector,
> str_enable_disable(enable));
> }
>
> +static int intel_dp_dsc_aux_ref_count(struct intel_atomic_state *state,
> + const struct intel_connector *connector,
> + bool for_get_ref)
> +{
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + struct drm_connector *_connector_iter;
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector_state *new_conn_state;
> + int ref_count = 0;
> + int i;
> +
> + /*
> + * On SST the decompression AUX device won't be shared, each connector
> + * uses for this its own AUX targeting the sink device.
> + */
> + if (!connector->mst_port)
> + return connector->dp.dsc_decompression_enabled ? 1 : 0;
> +
> + for_each_oldnew_connector_in_state(&state->base, _connector_iter,
> +old_conn_state, new_conn_state, i) {
> + const struct intel_connector *
> + connector_iter = to_intel_connector(_connector_iter);
> +
> + if (connector_iter->mst_port != connector->mst_port)
> + continue;
> +
> + if (!connector_iter->dp.dsc_decompression_enabled)
> + continue;
> +
> + drm_WARN_ON(&i915->drm,
> + (for_get_ref && !new_conn_state->crtc) ||
> + (!for_get_ref && !old_conn_state->crtc));
> +
> + if (connector_iter->dp.dsc_decompression_aux ==
> + connector->dp.dsc_decompression_aux)
> + ref_count++;
> + }
> +
> + return ref_count;
> +}
> +
> +static bool intel_dp_dsc_aux_get_ref(struct intel_atomic_state *state,
> + struct intel_connector *connector)
> +{
> + bool ret = intel_dp_dsc_aux_ref_count(state, connector, true) == 0;
> +
> + connector->dp.dsc_decompression_enabled = true;
> +
> + return ret;
> +}
> +
> +static bool intel_dp_dsc_aux_put_ref(struct intel_atomic_state *state,
> + struct intel_connector *connector)
> +{
> + connector->dp.dsc_decompression_enabled = false;
> +
> + return intel_dp_dsc_aux_ref_count(state, connector, false) == 0;
> +}
> +
>
Re: [Intel-gfx] [PATCH] drm/i915/display: assume some pixelrate for src smaller than 1
On Wed, Jan 11, 2023 at 09:39:26PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 11, 2023 at 11:28:51AM -0700, Drew Davenport wrote:
> > On Wed, Jan 11, 2023 at 04:19:00PM +0200, Ville Syrjälä wrote:
> > > On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote:
> > > > intel_adjusted_rate() didn't take into account src rectangle
> > > > can be less than 1 in width or height.
> > >
> > > This should not get called in those cases. What does the
> > > backtrace look like?
> >
> > In my repro of this issue, the backtrace looks as follows:
> >
> > [ 180.798331] RIP: 0010:intel_plane_pixel_rate+0x4a/0x53
> > [ 180.798336] Code:
> > [ 180.798338] RSP: 0018:b080ce4179b8 EFLAGS: 00010246
> > [ 180.798341] RAX: RBX: 98cd22a24000 RCX:
> > 0a00
> > [ 180.798343] RDX: RSI: 98cccbae7000 RDI:
> >
> > [ 180.798346] RBP: b080ce4179b8 R08: 00087780 R09:
> > 0002
> > [ 180.798348] R10: 0a00 R11: R12:
> >
> > [ 180.798350] R13: 98cd0e495400 R14: 98ccc34e R15:
> > 98cccbae7000
> > [ 180.798352] FS: 7b84119b5000() GS:98d02f90()
> > knlGS:
> > [ 180.798354] CS: 0010 DS: ES: CR0: 80050033
> > [ 180.798357] CR2: 7ffc2d5e4080 CR3: 0001042ee006 CR4:
> > 00770ee0
> > [ 180.798359] PKRU: 5554
> > [ 180.798361] Call Trace:
> > [ 180.798364]
> > [ 180.798366] intel_plane_atomic_check_with_state+0x1fd/0x6ea
> > [ 180.798370] ? intel_plane_atomic_check+0x11b/0x145
> > [ 180.798373] intel_atomic_check_planes+0x263/0x7ce
> > [ 180.798376] ? drm_atomic_helper_check_modeset+0x189/0x923
> > [ 180.798380] intel_atomic_check+0x14e4/0x184d
> > [ 180.798382] ? intel_rps_mark_interactive+0x23/0x6a
> > [ 180.798386] drm_atomic_check_only+0x3ec/0x98f
> > [ 180.798391] drm_atomic_commit+0xa2/0x105
> > [ 180.798394] ? drm_atomic_set_fb_for_plane+0x96/0xa5
> > [ 180.798397] drm_atomic_helper_update_plane+0xdc/0x11f
> > [ 180.798400] drm_mode_setplane+0x236/0x30c
> > [ 180.798404] ? drm_any_plane_has_format+0x51/0x51
> > [ 180.798407] drm_ioctl_kernel+0xda/0x14d
> > [ 180.798411] drm_ioctl+0x27e/0x3b4
> > [ 180.798414] ? drm_any_plane_has_format+0x51/0x51
> > [ 180.798418] __se_sys_ioctl+0x7a/0xbc
> > [ 180.798421] do_syscall_64+0x55/0x9d
> > [ 180.798424] ? exit_to_user_mode_prepare+0x3c/0x8b
> > [ 180.798427] entry_SYSCALL_64_after_hwframe+0x61/0xcb
> >
> > If this function shouldn't be called in such a case, then perhaps
> > I should revist my original attempt at fixing this in
> > https://patchwork.freedesktop.org/patch/516060 by rejecting such a
> > configuration?
>
> I'm saying that this should be impossible already. At least
> I can't immediately see anything that could call this with
> an invisible plane.
In my repro case, I called drmModeSetPlane with the src_h parameter set
to 65535 (so the largest 16.16 number that's less than one). This got
through any existing checks on the height of the src rect, resulting in
the divide-by-zero error in intel_plane_pixel_rate.
While investigating this, I tried setting src_h to 0, but this
configuration got rejected somewhere along the line before it got
through the intel_plane_pixel_rate.
>
> >
> > I'll respond to Alan on that thread.
> >
> > >
> > > >
> > > > Signed-off-by: Juha-Pekka Heikkila
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +---
> > > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > index 10e1fc9d0698..a9948e8d3543 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct
> > > > drm_rect *src,
> > > > const struct drm_rect *dst,
> > > > unsigned int rate)
> > > > {
> > > > - unsigned int src_w, src_h, dst_w, dst_h;
> > > > + unsigned int src_w, src_h, dst_w, dst_h, dst_wh;
> > > >
> > > > src_w = drm
Re: [Intel-gfx] [PATCH] drm/i915/display: Check source height is > 0
On Tue, Dec 27, 2022 at 05:55:17PM +, Teres Alexis, Alan Previn wrote:
> Is there a better place for this check higher up the intel specific
> atomic-check? (so the check won't be skl specific - i notice that
> intel_adjusted_rate is also called by
> ilk_foo as well and non-backend-specific functions). Else, perhaps
> intel_adjusted_rate should add a check + WARN? (if we are trying to propagate
> this slowly across HW).
Would intel_plane_atomic_check_with_state be a more appropriate
place to check that the src width and height are at least 1? This is
where skl_plane_check and other HW's foo_plane_check functions are called
from.
I don't think that the potential divide-by-zero will be hit in the case
where intel_adjusted_rate is called from ilk_pipe_pixel_rate because the
src rect will not have a fractional part to it in this case. I'm assuming
that something earlier on would catch and reject a src with zero
width/height.
Drew
>
>
> ...alan
>
> On Mon, 2022-12-26 at 22:53 -0700, Drew Davenport wrote:
> > The error message suggests that the height of the src rect must be at
> > least 1. Reject source with height of 0.
> >
> > Signed-off-by: Drew Davenport
> >
> > ---
> > I was investigating some divide-by-zero crash reports on ChromeOS which
> > pointed to the intel_adjusted_rate function. Further prodding showed
> > that I could reproduce this in a simple test program if I made src_h
> > some value less than 1 but greater than 0.
> >
> > This seemed to be a sensible place to check that the source height is at
> > least 1. I tried to repro this issue on an amd device I had on hand, and
> > the configuration was rejected.
> >
> > Would it make sense to add a check that source dimensions are at least 1
> > somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> > use case on some devices, and thus any such check should be done on a
> > per-driver basis?
> >
> > Thanks.
> >
> > drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 4b79c2d2d6177..9b172a1e90deb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct
> > intel_plane_state *plane_state)
> > u32 offset;
> > int ret;
> >
> > - if (w > max_width || w < min_width || h > max_height) {
> > + if (w > max_width || w < min_width || h > max_height || h < 1) {
> > drm_dbg_kms(&dev_priv->drm,
> > "requested Y/RGB source size %dx%d outside limits
> > (min: %dx1 max: %dx%d)\n",
> > w, h, min_width, max_width, max_height);
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >
>
Re: [Intel-gfx] [PATCH] drm/i915/display: assume some pixelrate for src smaller than 1
On Wed, Jan 11, 2023 at 04:19:00PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote:
> > intel_adjusted_rate() didn't take into account src rectangle
> > can be less than 1 in width or height.
>
> This should not get called in those cases. What does the
> backtrace look like?
In my repro of this issue, the backtrace looks as follows:
[ 180.798331] RIP: 0010:intel_plane_pixel_rate+0x4a/0x53
[ 180.798336] Code:
[ 180.798338] RSP: 0018:b080ce4179b8 EFLAGS: 00010246
[ 180.798341] RAX: RBX: 98cd22a24000 RCX: 0a00
[ 180.798343] RDX: RSI: 98cccbae7000 RDI:
[ 180.798346] RBP: b080ce4179b8 R08: 00087780 R09: 0002
[ 180.798348] R10: 0a00 R11: R12:
[ 180.798350] R13: 98cd0e495400 R14: 98ccc34e R15: 98cccbae7000
[ 180.798352] FS: 7b84119b5000() GS:98d02f90()
knlGS:
[ 180.798354] CS: 0010 DS: ES: CR0: 80050033
[ 180.798357] CR2: 7ffc2d5e4080 CR3: 0001042ee006 CR4: 00770ee0
[ 180.798359] PKRU: 5554
[ 180.798361] Call Trace:
[ 180.798364]
[ 180.798366] intel_plane_atomic_check_with_state+0x1fd/0x6ea
[ 180.798370] ? intel_plane_atomic_check+0x11b/0x145
[ 180.798373] intel_atomic_check_planes+0x263/0x7ce
[ 180.798376] ? drm_atomic_helper_check_modeset+0x189/0x923
[ 180.798380] intel_atomic_check+0x14e4/0x184d
[ 180.798382] ? intel_rps_mark_interactive+0x23/0x6a
[ 180.798386] drm_atomic_check_only+0x3ec/0x98f
[ 180.798391] drm_atomic_commit+0xa2/0x105
[ 180.798394] ? drm_atomic_set_fb_for_plane+0x96/0xa5
[ 180.798397] drm_atomic_helper_update_plane+0xdc/0x11f
[ 180.798400] drm_mode_setplane+0x236/0x30c
[ 180.798404] ? drm_any_plane_has_format+0x51/0x51
[ 180.798407] drm_ioctl_kernel+0xda/0x14d
[ 180.798411] drm_ioctl+0x27e/0x3b4
[ 180.798414] ? drm_any_plane_has_format+0x51/0x51
[ 180.798418] __se_sys_ioctl+0x7a/0xbc
[ 180.798421] do_syscall_64+0x55/0x9d
[ 180.798424] ? exit_to_user_mode_prepare+0x3c/0x8b
[ 180.798427] entry_SYSCALL_64_after_hwframe+0x61/0xcb
If this function shouldn't be called in such a case, then perhaps
I should revist my original attempt at fixing this in
https://patchwork.freedesktop.org/patch/516060 by rejecting such a
configuration?
I'll respond to Alan on that thread.
>
> >
> > Signed-off-by: Juha-Pekka Heikkila
> > ---
> > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 10e1fc9d0698..a9948e8d3543 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> > *src,
> > const struct drm_rect *dst,
> > unsigned int rate)
> > {
> > - unsigned int src_w, src_h, dst_w, dst_h;
> > + unsigned int src_w, src_h, dst_w, dst_h, dst_wh;
> >
> > src_w = drm_rect_width(src) >> 16;
> > src_h = drm_rect_height(src) >> 16;
> > @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> > *src,
> > dst_w = min(src_w, dst_w);
> > dst_h = min(src_h, dst_h);
> >
> > - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
> > - dst_w * dst_h);
> > + /* in case src contained only fractional part */
> > + dst_wh = max(dst_w * dst_h, (unsigned) 1);
> > +
> > + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh);
> > }
> >
> > unsigned int intel_plane_pixel_rate(const struct intel_crtc_state
> > *crtc_state,
> > --
> > 2.37.3
>
> --
> Ville Syrjälä
> Intel
Re: [Intel-gfx] [PATCH] drm/i915/display: Check source height is > 0
On Tue, Jan 03, 2023 at 12:42:43PM +0200, Juha-Pekka Heikkila wrote:
> Hi Drew,
Hi Juha-Pekka, sorry for the late response since I was on vacation.
>
> this is good find. I went looking where the problem is in and saw what you
> probably also saw earlier.
>
> I was wondering if diff below would be better fix? I assume this would end
> up with einval or erange in your case but code flow otherwise would stay as
> is while fixing all future callers for same issue:
Yes, the function you identify below is where I encountered
divide-by-zero errors. If width/height less than 1 is a legitimate use
case, then your proposed solution looks like it would be better. It
should have no risk of regression in userspace either, which is nice.
I tested your patch, and as expected I did not hit the divide-by-zero
error, though the test commit was rejected due to a check further along
inside skl_update_scaler. Perhaps there is some other configuration
which would pass the test commit with a width/height less than 1, but I
didn't dig much further.
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 10e1fc9d0698..a9948e8d3543 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> *src,
> const struct drm_rect *dst,
> unsigned int rate)
> {
> - unsigned int src_w, src_h, dst_w, dst_h;
> + unsigned int src_w, src_h, dst_w, dst_h, dst_wh;
>
> src_w = drm_rect_width(src) >> 16;
> src_h = drm_rect_height(src) >> 16;
> @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect
> *src,
> dst_w = min(src_w, dst_w);
> dst_h = min(src_h, dst_h);
>
> - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
> - dst_w * dst_h);
> + /* in case src contained only fractional part */
> + dst_wh = max(dst_w * dst_h, (unsigned) 1);
> +
> + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh);
> }
>
> unsigned int intel_plane_pixel_rate(const struct intel_crtc_state
> *crtc_state,
>
>
> What do you think? I'll in any case come up with some test for this in igt.
I see that you've posted your fix to the list already. Adding a
test to cover this in IGT also sounds great. Thanks!
Breadcrumbs to Juha-Pekka's patch for anyone following this
thread: https://patchwork.freedesktop.org/series/112396/
>
> /Juha-Pekka
>
> On 27.12.2022 7.53, Drew Davenport wrote:
> > The error message suggests that the height of the src rect must be at
> > least 1. Reject source with height of 0.
> >
> > Signed-off-by: Drew Davenport
> >
> > ---
> > I was investigating some divide-by-zero crash reports on ChromeOS which
> > pointed to the intel_adjusted_rate function. Further prodding showed
> > that I could reproduce this in a simple test program if I made src_h
> > some value less than 1 but greater than 0.
> >
> > This seemed to be a sensible place to check that the source height is at
> > least 1. I tried to repro this issue on an amd device I had on hand, and
> > the configuration was rejected.
> >
> > Would it make sense to add a check that source dimensions are at least 1
> > somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> > use case on some devices, and thus any such check should be done on a
> > per-driver basis?
> >
> > Thanks.
> >
> > drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 4b79c2d2d6177..9b172a1e90deb 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct
> > intel_plane_state *plane_state)
> > u32 offset;
> > int ret;
> > - if (w > max_width || w < min_width || h > max_height) {
> > + if (w > max_width || w < min_width || h > max_height || h < 1) {
> > drm_dbg_kms(&dev_priv->drm,
> > "requested Y/RGB source size %dx%d outside limits
> > (min: %dx1 max: %dx%d)\n",
> > w, h, min_width, max_width, max_height);
>
[Intel-gfx] [PATCH] drm/i915/display: Check source height is > 0
The error message suggests that the height of the src rect must be at
least 1. Reject source with height of 0.
Signed-off-by: Drew Davenport
---
I was investigating some divide-by-zero crash reports on ChromeOS which
pointed to the intel_adjusted_rate function. Further prodding showed
that I could reproduce this in a simple test program if I made src_h
some value less than 1 but greater than 0.
This seemed to be a sensible place to check that the source height is at
least 1. I tried to repro this issue on an amd device I had on hand, and
the configuration was rejected.
Would it make sense to add a check that source dimensions are at least 1
somewhere in core, like in drm_atomic_plane_check? Or is that a valid
use case on some devices, and thus any such check should be done on a
per-driver basis?
Thanks.
drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 4b79c2d2d6177..9b172a1e90deb 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct
intel_plane_state *plane_state)
u32 offset;
int ret;
- if (w > max_width || w < min_width || h > max_height) {
+ if (w > max_width || w < min_width || h > max_height || h < 1) {
drm_dbg_kms(&dev_priv->drm,
"requested Y/RGB source size %dx%d outside limits
(min: %dx1 max: %dx%d)\n",
w, h, min_width, max_width, max_height);
--
2.39.0.314.g84b9a713c41-goog
Re: [Intel-gfx] [v7 1/5] drm/edid: seek for available CEA and DisplayID block from specific EDID block index
On Tue, Mar 15, 2022 at 03:21:05PM +, Lee, Shawn C wrote:
> On Tuesday, March 15, 2022 8:33 PM, Nikula, Jani
> wrote:
> >On Mon, 14 Mar 2022, Drew Davenport wrote:
> >> On Mon, Mar 14, 2022 at 10:40:47AM +0200, Jani Nikula wrote:
> >>> On Sun, 13 Mar 2022, Lee Shawn C wrote:
> >>> > drm_find_cea_extension() always look for a top level CEA block.
> >>> > Pass ext_index from caller then this function to search next
> >>> > available CEA ext block from a specific EDID block pointer.
> >>> >
> >>> > v2: save proper extension block index if CTA data information
> >>> > was found in DispalyID block.
> >>> > v3: using different parameters to store CEA and DisplayID block index.
> >>> > configure DisplayID extansion block index before search available
> >>> > DisplayID block.
> >>> >
> >>> > Cc: Jani Nikula
> >>> > Cc: Ville Syrjala
> >>> > Cc: Ankit Nautiyal
> >>> > Cc: Drew Davenport
> >>> > Cc: intel-gfx
> >>> > Signed-off-by: Lee Shawn C
> >>> > ---
> >>> > drivers/gpu/drm/drm_displayid.c | 10 +--
> >>> > drivers/gpu/drm/drm_edid.c | 53 ++---
> >>> > include/drm/drm_displayid.h | 4 +--
> >>> > 3 files changed, 39 insertions(+), 28 deletions(-)
> >>> >
> >>> > diff --git a/drivers/gpu/drm/drm_displayid.c
> >>> > b/drivers/gpu/drm/drm_displayid.c index 32da557b960f..31c3e6d7d549
> >>> > 100644
> >>> > --- a/drivers/gpu/drm/drm_displayid.c
> >>> > +++ b/drivers/gpu/drm/drm_displayid.c
> >>> > @@ -59,11 +59,14 @@ static const u8
> >>> > *drm_find_displayid_extension(const struct edid *edid, }
> >>> >
> >>> > void displayid_iter_edid_begin(const struct edid *edid,
> >>> > - struct displayid_iter *iter)
> >>> > + struct displayid_iter *iter, int
> >>> > *ext_index)
> >>>
> >>> Please don't do this. This just ruins the clean approach displayid
> >>> iterator added.
> >>>
> >>> Instead of making the displayid iterator ugly, and leaking its
> >>> abstractions, I'll repeat what I said should be done in reply to the
> >>> very first version of this patch series [1]:
> >>>
> >>> "I think we're going to need abstracted EDID iteration similar to
> >>> what I've done for DisplayID iteration. We can't have all places
> >>> reimplementing the iteration like we have now."
> >>>
> >>> This isn't a problem that should be solved by having all the callers
> >>> hold a bunch of local variables and pass them around to all the
> >>> functions. Nobody's going to be able to keep track of this anymore.
> >>> And this series, as it is, makes it harder to fix this properly later on.
> >>
> >> I missed your original review comment, so apologies for repeating what
> >> you said there already.
> >>
> >> I'd agree that passing a starting index to the displayid_iter_*
> >> functions is probably not the right direction here. More thoughts below.
> >>
> >>>
> >>>
> >>> BR,
> >>> Jani.
> >>>
> >>>
> >>> [1] https://lore.kernel.org/r/87czjf8dik@intel.com
> >>>
> >>>
> >>>
> >>> > {
> >>> > memset(iter, 0, sizeof(*iter));
> >>> >
> >>> > iter->edid = edid;
> >>> > +
> >>> > + if (ext_index)
> >>> > + iter->ext_index = *ext_index;
> >>> > }
> >>> >
> >>> > static const struct displayid_block * @@ -126,7 +129,10 @@
> >>> > __displayid_iter_next(struct displayid_iter *iter)
> >>> > }
> >>> > }
> >>> >
> >>> > -void displayid_iter_end(struct displayid_iter *iter)
> >>> > +void displayid_iter_end(struct displayid_iter *iter, int
> >>> > +*ext_index)
> >>> > {
> >>> > + if (ext_index)
> >>> > + *ext_index = iter->ext_index;
> >>> > +
> >>
Re: [Intel-gfx] [v7 1/5] drm/edid: seek for available CEA and DisplayID block from specific EDID block index
On Mon, Mar 14, 2022 at 10:40:47AM +0200, Jani Nikula wrote:
> On Sun, 13 Mar 2022, Lee Shawn C wrote:
> > drm_find_cea_extension() always look for a top level CEA block. Pass
> > ext_index from caller then this function to search next available
> > CEA ext block from a specific EDID block pointer.
> >
> > v2: save proper extension block index if CTA data information
> > was found in DispalyID block.
> > v3: using different parameters to store CEA and DisplayID block index.
> > configure DisplayID extansion block index before search available
> > DisplayID block.
> >
> > Cc: Jani Nikula
> > Cc: Ville Syrjala
> > Cc: Ankit Nautiyal
> > Cc: Drew Davenport
> > Cc: intel-gfx
> > Signed-off-by: Lee Shawn C
> > ---
> > drivers/gpu/drm/drm_displayid.c | 10 +--
> > drivers/gpu/drm/drm_edid.c | 53 ++---
> > include/drm/drm_displayid.h | 4 +--
> > 3 files changed, 39 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_displayid.c
> > b/drivers/gpu/drm/drm_displayid.c
> > index 32da557b960f..31c3e6d7d549 100644
> > --- a/drivers/gpu/drm/drm_displayid.c
> > +++ b/drivers/gpu/drm/drm_displayid.c
> > @@ -59,11 +59,14 @@ static const u8 *drm_find_displayid_extension(const
> > struct edid *edid,
> > }
> >
> > void displayid_iter_edid_begin(const struct edid *edid,
> > - struct displayid_iter *iter)
> > + struct displayid_iter *iter, int *ext_index)
>
> Please don't do this. This just ruins the clean approach displayid
> iterator added.
>
> Instead of making the displayid iterator ugly, and leaking its
> abstractions, I'll repeat what I said should be done in reply to the
> very first version of this patch series [1]:
>
> "I think we're going to need abstracted EDID iteration similar to what
> I've done for DisplayID iteration. We can't have all places
> reimplementing the iteration like we have now."
>
> This isn't a problem that should be solved by having all the callers
> hold a bunch of local variables and pass them around to all the
> functions. Nobody's going to be able to keep track of this anymore. And
> this series, as it is, makes it harder to fix this properly later on.
I missed your original review comment, so apologies for repeating what you
said there already.
I'd agree that passing a starting index to the displayid_iter_*
functions is probably not the right direction here. More thoughts below.
>
>
> BR,
> Jani.
>
>
> [1] https://lore.kernel.org/r/87czjf8dik@intel.com
>
>
>
> > {
> > memset(iter, 0, sizeof(*iter));
> >
> > iter->edid = edid;
> > +
> > + if (ext_index)
> > + iter->ext_index = *ext_index;
> > }
> >
> > static const struct displayid_block *
> > @@ -126,7 +129,10 @@ __displayid_iter_next(struct displayid_iter *iter)
> > }
> > }
> >
> > -void displayid_iter_end(struct displayid_iter *iter)
> > +void displayid_iter_end(struct displayid_iter *iter, int *ext_index)
> > {
> > + if (ext_index)
> > + *ext_index = iter->ext_index;
> > +
> > memset(iter, 0, sizeof(*iter));
> > }
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 561f53831e29..78c415aa6889 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3353,28 +3353,27 @@ const u8 *drm_find_edid_extension(const struct edid
> > *edid,
> > return edid_ext;
> > }
> >
> > -static const u8 *drm_find_cea_extension(const struct edid *edid)
> > +static const u8 *drm_find_cea_extension(const struct edid *edid,
> > + int *cea_ext_index, int
> > *displayid_ext_index)
As discussed above, passing both indices into this function may not be
the best approach here. But I think we need to keep track of some kind
of state in order to know which was the last CEA block that was
returned, and thus this function can return the next one after that,
whether it's in the CEA extension block or DisplayID block.
What do you think of using the pointer returned from this function as
that state? The caller could pass in a u8* that was returned from a
previous call. This function would iterate through the extension blocks
and skip the CEA blocks it finds until it finds the passed in pointer,
and then return the next one after (or NULL).
An alternative might be to create a linked list of pt
Re: [Intel-gfx] [v6 1/5] drm/edid: seek for available CEA block from specific EDID block index
On Fri, Mar 11, 2022 at 09:22:14AM +0800, Lee Shawn C wrote:
> drm_find_cea_extension() always look for a top level CEA block. Pass
> ext_index from caller then this function to search next available
> CEA ext block from a specific EDID block pointer.
>
> v2: save proper extension block index if CTA data information
> was found in DispalyID block.
>
> Cc: Jani Nikula
> Cc: Ville Syrjala
> Cc: Ankit Nautiyal
> Cc: intel-gfx
> Signed-off-by: Lee Shawn C
> ---
> drivers/gpu/drm/drm_edid.c | 43 +++---
> 1 file changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 561f53831e29..e267d31d5c87 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3353,16 +3353,14 @@ const u8 *drm_find_edid_extension(const struct edid
> *edid,
> return edid_ext;
> }
>
> -static const u8 *drm_find_cea_extension(const struct edid *edid)
> +static const u8 *drm_find_cea_extension(const struct edid *edid, int
> *ext_index)
> {
> const struct displayid_block *block;
> struct displayid_iter iter;
> const u8 *cea;
> - int ext_index = 0;
>
> - /* Look for a top level CEA extension block */
> - /* FIXME: make callers iterate through multiple CEA ext blocks? */
> - cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
> + /* Look for a CEA extension block from ext_index */
> + cea = drm_find_edid_extension(edid, CEA_EXT, ext_index);
> if (cea)
> return cea;
>
> @@ -3370,6 +3368,7 @@ static const u8 *drm_find_cea_extension(const struct
> edid *edid)
> displayid_iter_edid_begin(edid, &iter);
> displayid_iter_for_each(block, &iter) {
> if (block->tag == DATA_BLOCK_CTA) {
> + *ext_index = iter.ext_index;
This could still end up in an infinite loop in patch 2 in the case that
there is no CEA_EXT block in the edid, but there is a CEA block in the
DisplayId block.
Repeating my review comment from elsewhere, consider the case:
- If there are no cea extension blocks in the EDID,
drm_find_edid_extension returns NULL
- drm_find_cea_extension will then return the first DisplayId block
with tag DATA_BLOCK_CTA
If the version of the cea data from DisplayId block is less than 3, the
loop will restart and call drm_find_cea_extension the same way, returning
the same DisplayID block every time.
Setting *ext_index inside the display_iter_for_each block doesn't change this,
since we're not checking it.
But I don't think we want to use the same *ext_index both to pass into
drm_find_edid_extension and for tracking the next DisplayId block to check.
This might end up in similar infinite loops or skipping DisplayId blocks.
Maybe you'll need to pass in two indexes to drm_find_cea_extension, one which
is passed to drm_find_edid_extension, and the other to keep track of the next
DisplayId block to check.
> cea = (const u8 *)block;
> break;
> }
> @@ -3643,10 +3642,10 @@ add_alternate_cea_modes(struct drm_connector
> *connector, struct edid *edid)
> struct drm_device *dev = connector->dev;
> struct drm_display_mode *mode, *tmp;
> LIST_HEAD(list);
> - int modes = 0;
> + int modes = 0, ext_index = 0;
>
> /* Don't add CEA modes if the CEA extension block is missing */
> - if (!drm_find_cea_extension(edid))
> + if (!drm_find_cea_extension(edid, &ext_index))
> return 0;
>
> /*
> @@ -4321,11 +4320,11 @@ static void drm_parse_y420cmdb_bitmap(struct
> drm_connector *connector,
> static int
> add_cea_modes(struct drm_connector *connector, struct edid *edid)
> {
> - const u8 *cea = drm_find_cea_extension(edid);
> - const u8 *db, *hdmi = NULL, *video = NULL;
> + const u8 *cea, *db, *hdmi = NULL, *video = NULL;
> u8 dbl, hdmi_len, video_len = 0;
> - int modes = 0;
> + int modes = 0, ext_index = 0;
>
> + cea = drm_find_cea_extension(edid, &ext_index);
> if (cea && cea_revision(cea) >= 3) {
> int i, start, end;
>
> @@ -4562,7 +4561,7 @@ static void drm_edid_to_eld(struct drm_connector
> *connector, struct edid *edid)
> uint8_t *eld = connector->eld;
> const u8 *cea;
> const u8 *db;
> - int total_sad_count = 0;
> + int total_sad_count = 0, ext_index = 0;
> int mnl;
> int dbl;
>
> @@ -4571,7 +4570,7 @@ static void drm_edid_to_eld(struct drm_connector
> *connector, struct edid *edid)
> if (!edid)
> return;
>
> - cea = drm_find_cea_extension(edid);
> + cea = drm_find_cea_extension(edid, &ext_index);
> if (!cea) {
> DRM_DEBUG_KMS("ELD: no CEA Extension found\n");
> return;
> @@ -4655,11 +4654,11 @@ static void drm_edid_to_eld(struct drm_connector
> *connector, struct edid *edid)
> */
> int drm_edid_to_sad(struct edid *edid, st
