Re: [PATCH v2 02/21] drm/dp: Add support for DP tunneling
On Fri, Feb 23, 2024 at 11:32:21PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 20, 2024 at 11:18:22PM +0200, Imre Deak wrote:
> > +static inline void drm_dp_tunnel_ref_put(struct drm_dp_tunnel_ref
> > *tunnel_ref)
> > +{
> > + drm_dp_tunnel_put(tunnel_ref->tunnel, &tunnel_ref->tracker);
>
> Should we set tunnel_ref->tunnel=NULL here?
Yes, thanks for spotting this. It also fixes
intel_crtc_prepare_cleared_state()->
intel_dp_tunnel_atomic_clear_stream_bw()
if crtc_state::dp_tunnel_ref state doesn't get recomputed, for instance
when disabling the crtc.
>
> --
> Ville Syrjälä
> Intel
Re: [PATCH v2 02/21] drm/dp: Add support for DP tunneling
On Tue, Feb 20, 2024 at 11:18:22PM +0200, Imre Deak wrote:
> +static inline void drm_dp_tunnel_ref_put(struct drm_dp_tunnel_ref
> *tunnel_ref)
> +{
> + drm_dp_tunnel_put(tunnel_ref->tunnel, &tunnel_ref->tracker);
Should we set tunnel_ref->tunnel=NULL here?
--
Ville Syrjälä
Intel
Re: [PATCH v2 02/21] drm/dp: Add support for DP tunneling
On Fri, Feb 23, 2024 at 08:25:38AM +0200, Shankar, Uma wrote:
> [...]
> > diff --git a/drivers/gpu/drm/display/Kconfig
> > b/drivers/gpu/drm/display/Kconfig
> > index 09712b88a5b83..c0f56888c3280 100644
> > --- a/drivers/gpu/drm/display/Kconfig
> > +++ b/drivers/gpu/drm/display/Kconfig
> [...]
> > +struct drm_dp_tunnel {
> > + struct drm_dp_tunnel_group *group;
> > +
> > + struct list_head node;
> > +
> > + struct kref kref;
> > + struct ref_tracker *tracker;
> > + struct drm_dp_aux *aux;
> > + char name[8];
> > +
> > + int bw_granularity;
> > + int estimated_bw;
> > + int allocated_bw;
> > +
> > + int max_dprx_rate;
> > + u8 max_dprx_lane_count;
> > +
> > + u8 adapter_id;
> > +
> > + bool bw_alloc_supported:1;
> > + bool bw_alloc_enabled:1;
> > + bool has_io_error:1;
> > + bool destroyed:1;
> > +};
> > +
> > +struct drm_dp_tunnel_group_state;
> > +
> > +struct drm_dp_tunnel_state {
> > + struct drm_dp_tunnel_group_state *group_state;
> > +
> > + struct drm_dp_tunnel_ref tunnel_ref;
> > +
> > + struct list_head node;
> > +
> > + u32 stream_mask;
> > + int *stream_bw;
> > +};
> > +
> > +struct drm_dp_tunnel_group_state {
> > + struct drm_private_state base;
> > +
> > + struct list_head tunnel_states;
> > +};
> > +
> > +struct drm_dp_tunnel_group {
> > + struct drm_private_obj base;
> > + struct drm_dp_tunnel_mgr *mgr;
> > +
> > + struct list_head tunnels;
> > +
> > + /* available BW including the allocated_bw of all tunnels in the
> > group */
> > + int available_bw;
> > + u8 drv_group_id;
> > +
> > + char name[8];
> > +
>
> We can drop these line gaps.
Those are used in general to keep related fields together, but the above
three fields could be better grouped, can do that.
> > + bool active:1;
> > +};
> > +
> > +struct drm_dp_tunnel_mgr {
> > + struct drm_device *dev;
> > +
> > + int group_count;
> > + struct drm_dp_tunnel_group *groups;
> > + wait_queue_head_t bw_req_queue;
> > +
> > +#ifdef CONFIG_DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE
> > + struct ref_tracker_dir ref_tracker;
> > +#endif
> > +};
> > +
> > + [...]
> > +/* Return granularity in kB/s units */
> > +static int tunnel_reg_bw_granularity(const struct drm_dp_tunnel_regs *regs)
> > +{
> > + int gr = tunnel_reg(regs, DP_BW_GRANULARITY) &
> > DP_BW_GRANULARITY_MASK;
> > +
> > + WARN_ON(gr > 2);
> > +
>
> I think it should fail as well along with WARN, > 2 is not supported.
> However, this situation is highly unlikely though.
Yes, validating this in tunnel_regs_are_valid() is missing, will add
that.
> > + return (25 << gr) / 8;
> [...]
> > +
> > +static int check_and_clear_status_change(struct drm_dp_tunnel *tunnel)
> > +{
> > + u8 mask = DP_BW_ALLOCATION_CAPABILITY_CHANGED |
> > DP_ESTIMATED_BW_CHANGED;
> > + u8 val;
> > +
> > + if (drm_dp_dpcd_readb(tunnel->aux, DP_TUNNELING_STATUS, &val) < 0)
> > + goto out_err;
> > +
> > + val &= mask;
> > +
> > + if (val) {
> > + if (drm_dp_dpcd_writeb(tunnel->aux, DP_TUNNELING_STATUS, val)
> > < 0)
> > + goto out_err;
> > +
> > + return 1;
>
> Seems this return value is not considered by the caller.
It indicates a status change and handled accordingly by the caller.
Probably it's better to document this, can add that.
> Hi Imre,
> Overall changes look good to me.
> It would be ideal if we can break this patch down, no major concern though.
> Leaving to your judgment.
The current way adds the DRM/i915 specific support separately, each
adding one .c file, the rest of the patchset taking these into use, each
patch doing this for a given scope of functionality. Didn't have a
better idea, maybe the DPCD registers could be added separately, but not
sure if that would make reviewing easier.
> With some above minor nits addressed, this is
> Reviewed-by: Uma Shankar
>
> Note: Have checked the register definitions/addresses, offsets. Have
> Logically checked the code
> in general, as well. It would be good if tunnel manager and tunnel groups
> related logic can be double
> confirmed by someone. Having said that, no issue I could spot there.
Thanks.
> Regards,
> Uma Shankar
>
> > + }
> > +
> > + if (!drm_dp_tunnel_bw_alloc_is_enabled(tunnel))
> > + return 0;
> > +
> > + /*
> > + * Check for estimated BW changes explicitly to account for lost
> > + * BW change notifications.
> > + */
> > + if (drm_dp_dpcd_readb(tunnel->aux, DP_ESTIMATED_BW, &val) < 0)
> > + goto out_err;
> > +
> > + if (val * tunnel->bw_granularity != tunnel->estimated_bw)
> > + return 1;
> > +
> > + return 0;
> > +
> > +out_err:
> > + drm_dp_tunnel_set_io_error(tunnel);
> > +
> > + return -EIO;
> > +}
> > +
> > +/**
> > + * drm_dp_tunnel_update_state - Update DP tunnel SW state with the HW state
> > + * @tunnel: Tunnel obj
RE: [PATCH v2 02/21] drm/dp: Add support for DP tunneling
> -Original Message- > From: Intel-gfx On Behalf Of Imre > Deak > Sent: Wednesday, February 21, 2024 2:48 AM > To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Cc: Ville Syrjälä > Subject: [PATCH v2 02/21] drm/dp: Add support for DP tunneling > > Add support for Display Port tunneling. For now this includes the > support for Bandwidth Allocation Mode (BWA), leaving adding Panel Replay > support for later. > > BWA allows using displays that share the same (Thunderbolt) link with > their maximum resolution. Atm, this may not be possible due to the > coarse granularity of partitioning the link BW among the displays on the > link: the BW allocation policy is in a SW/FW/HW component on the link > (on Thunderbolt it's the SW or FW Connection Manager), independent of > the driver. This policy will set the DPRX maximum rate and lane count > DPCD registers the GFX driver will see (0x0, 0x1, 0x02200, > 0x02201) based on the available link BW. > > The granularity of the current BW allocation policy is coarse, based on > the required link rate in the 1.62Gbs..8.1Gbps range and it may prevent > using higher resolutions all together: the display connected first will > get a share of the link BW which corresponds to its full DPRX capability > (regardless of the actual mode it uses). A subsequent display connected > will only get the remaining BW, which could be well below its full > capability. > > BWA solves the above coarse granularity (reducing it to a 250Mbs..1Gps > range) and first-come/first-served issues by letting the driver request > the BW for each display on a link which reflects the actual modes the > displays use. > > This patch adds the DRM core helper functions, while a follow-up change > in the patchset takes them into use in the i915 driver. > > v2: > - Fix prepare_to_wait vs. wake-up cond check order in > allocate_tunnel_bw(). (Ville) > - Move tunnel==NULL checks from callers in drivers to here. (Ville) > - Avoid var inits in declaration blocks that can fail or have > side-effects. (Ville) > - Use u8 for driver and group IDs. (Ville) > - Simplify API removing drm_dp_tunnel_get/put_untracked(). (Ville) > - Reuse str_yes_no() instead of a local yes_no_chr(). (Ville) > - s/drm_dp_tunnel_atomic_clear_state()/free_tunnel_state() and unexport > the function. (Ville) > - s/clear_tunnel_group_state()/free_group_state() and move kfree() to > this function. (Ville) > - Add separate group_free_bw() helper and describe what the tunnel > estimated BW includes. (Ville) > - Improve help text for CONFIG_DRM_DISPLAY_DP_TUNNEL. (Ville) > - Add code comment explaining the purpose of DPCD reg read helpers. > (Ville) > - Add code comment describing the tunnel group name prefix format. > (Ville) > - Report the allocated BW as undetermined until the first allocation > request. > - Skip allocation requests matching the previous request. > - Clear any stale BW request status flags before a new request. > - Add missing error return check of drm_dp_tunnel_atomic_get_group_state() > in drm_dp_tunnel_atomic_set_stream_bw(). > - Add drm_dp_tunnel_get_allocated_bw(). > - > s/drm_dp_tunnel_atomic_get_tunnel_bw/drm_dp_tunnel_atomic_get_required_ > bw > - Fix return value description in function doc of drm_dp_tunnel_detect(). > - Add function documentation to all exported functions. > > Cc: Ville Syrjälä > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/display/Kconfig | 21 + > drivers/gpu/drm/display/Makefile|2 + > drivers/gpu/drm/display/drm_dp_tunnel.c | 1929 +++ > include/drm/display/drm_dp.h| 60 + > include/drm/display/drm_dp_tunnel.h | 248 +++ > 5 files changed, 2260 insertions(+) > create mode 100644 drivers/gpu/drm/display/drm_dp_tunnel.c > create mode 100644 include/drm/display/drm_dp_tunnel.h > > diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig > index 09712b88a5b83..c0f56888c3280 100644 > --- a/drivers/gpu/drm/display/Kconfig > +++ b/drivers/gpu/drm/display/Kconfig > @@ -17,6 +17,27 @@ config DRM_DISPLAY_DP_HELPER > help > DRM display helpers for DisplayPort. > > +config DRM_DISPLAY_DP_TUNNEL > + bool > + select DRM_DISPLAY_DP_HELPER > + help > + Enable support for DisplayPort tunnels. This allows drivers to use > + DP tunnel features like the Bandwidth Allocation mode to maximize the > + BW utilization for display streams on Thunderbolt links. > + > +config DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE > + bool "Enable debugging the DP tunnel state" > + depends on REF_TRACKER > + depends on DRM_DISPLAY_DP_TUNNEL > + depends on DEBUG_KERNEL > + depends on EXPERT > + help > + Enables debugging the DP tunnel manager's state, including the > + consistency of all managed tunnels' reference counting and the state > of > + streams contained in tunnels. > + > + If in doubt, say "N". >
