Hi All, Looking forward for more reviews comments and concerns if any, regarding this patch. Regards, Anusha >-----Original Message----- >From: Jim Bride [mailto:jim.bride@xxxxxxxxxxxxxxx] >Sent: Tuesday, September 6, 2016 11:57 AM >To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Subject: Re: [PATCH] drm/i915/dp/mst: Validate modes against the >available link bandwidth > >On Tue, Sep 06, 2016 at 06:39:12PM +0000, Srivatsa, Anusha wrote: >> Sending to the list again since Ville's review comment didn't hit the mailing list >last time. >> >> Regards, >> Anusha >> >> -----Original Message----- >> From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >> Sent: Monday, September 5, 2016 4:39 AM >> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx> >> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> Subject: Re: [PATCH] drm/i915/dp/mst: Validate modes >> against the available link bandwidth >> >> On Thu, Aug 18, 2016 at 05:11:31PM -0700, Anusha Srivatsa wrote: >> > Change intel_dp_mst_mode_valid() to use available link bandwidth >> > rather than the link's maximum supported bandwidth to evaluate >> > whether modes are legal for the current configuration. This takes >> > into account the fact that link bandwidth may already be dedicated >> > to other virtual channels. >> >> We can't do this. Results of mode_valid() aren't supposed to change like this >depending on what else is happening in the system. The only thing mode_valid() >tells us is whether it's possible to use the mode under *some* circumstances. >Only if the mode can't be used under *any* circumstances should it be filtered >out. >> >> If we wanted to change this, we'd at the very least have to synthesize hotplug >uevents whenever the conditions changed. But doing that would be a fairly big >behavioural change, so I'm not sure how people feel about it. It also doesn't >really solve the problem since eg. atomic can go directly from 0->n active pipes, >so there's no way to know upfront which modes we can pick for each pipe. >> > >I won't dispute that this won't help for all cases, but it does make hotplugs, at a >minimum, more sane. For that reason alone, I'd like to see this patch land. >Longer term I think we should look at how we can make user space and atomic >better handle MST (IMHO in multi-modeset operations on MST atomic should >ensure that the aggregate dotclock of the modes being set are less than the link >bandwidth that's configured.) I think that user space also needs to be more MST >aware and update what's available based on the current configuration at any >given time. I'm not sure what the precise solution should look like here, but I'd >think that what we want is for user space to fetch the list of available resolutions >based on the current configuration any time we plug or unplug a device on a DP >MST topology. In an ideal world it would be nice to have some sort of >information about the total link bandwidth, and how much bandwidth each mode >is taking up so that the user could have a guide to tweaking their setup in such a >way to maximize how they choose to configure their monitors based on the >available link bandwidth. > >Jim > > >> > >> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> >> > --- >> > drivers/gpu/drm/i915/intel_dp_mst.c | 12 +++++++++--- >> > 1 file changed, 9 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c >> > b/drivers/gpu/drm/i915/intel_dp_mst.c >> > index 629337d..39c58eb 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c >> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c >> > @@ -352,16 +352,22 @@ static enum drm_mode_status >> > intel_dp_mst_mode_valid(struct drm_connector *connector, >> > struct drm_display_mode *mode) >> > { >> > - int max_dotclk = to_i915(connector->dev)->max_dotclk_freq; >> > + int req_pbn = 0; >> > + int slots = 0; >> > + struct intel_connector *intel_connector = >to_intel_connector(connector); >> > + struct intel_dp *intel_dp = intel_connector->mst_port; >> > + struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr; >> > + >> > + req_pbn = drm_dp_calc_pbn_mode(mode->clock, 24); >> > + slots = drm_dp_find_vcpi_slots(mgr, req_pbn); >> > >> > - /* TODO - validate mode against available PBN for link */ >> > if (mode->clock < 10000) >> > return MODE_CLOCK_LOW; >> > >> > if (mode->flags & DRM_MODE_FLAG_DBLCLK) >> > return MODE_H_ILLEGAL; >> > >> > - if (mode->clock > max_dotclk) >> > + if (slots == -ENOSPC) >> > return MODE_CLOCK_HIGH; >> > >> > return MODE_OK; >> > -- >> > 2.7.4 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Ville Syrjälä >> Intel OTC >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
