[Intel-gfx] [PATCH] drm/i915/dp/mst: Validate modes against the available link bandwidth

[Srivatsa, Anusha]

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 at linux.intel.com]
>Sent: Tuesday, September 6, 2016 11:57 AM
>To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>Cc: intel-gfx at lists.freedesktop.org
>Subject: Re: [Intel-gfx] [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 at linux.intel.com]
>> Sent: Monday, September 5, 2016 4:39 AM
>> To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>> Cc: intel-gfx at lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [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 at intel.com>
>> > ---
>> >  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 at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx