Re: [PATCH v7 3/5] drm/i915: Check pixel rate for DP to VGA dongle

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 11, 2016 at 03:26:42PM +0300, Mika Kahola wrote:
> On Thu, 2016-08-11 at 12:56 +0200, Daniel Vetter wrote:
> > On Thu, Aug 11, 2016 at 01:51:42PM +0300, Ville Syrjälä wrote:
> > > On Thu, Aug 11, 2016 at 12:43:39PM +0300, Mika Kahola wrote:
> > > > On Thu, 2016-08-11 at 10:18 +0300, Ville Syrjälä wrote:
> > > > > On Mon, Aug 08, 2016 at 04:00:28PM +0300, Mika Kahola wrote:
> > > > > > Filter out a mode that exceeds the max pixel rate setting
> > > > > > for DP to VGA dongle. This is defined in DPCD register 0x81
> > > > > > if detailed cap info i.e. info field is 4 bytes long and
> > > > > > it is available for DP downstream port.
> > > > > > 
> > > > > > The register defines the pixel rate divided by 8 in MP/s.
> > > > > > 
> > > > > > v2: DPCD read outs and computation moved to drm (Ville, Daniel)
> > > > > > v3: Sink pixel rate computation moved to drm_dp_max_sink_dotclock()
> > > > > >     function (Daniel)
> > > > > > v4: Use of drm_dp_helper.c routines to compute max pixel clock (Ville)
> > > > > > v5: Use of intel_dp->downstream_ports to read out port capabilities.
> > > > > >     Code restructuring (Ville)
> > > > > > v6: Move DP branch device check to drm_dp_helper.c (Daniel)
> > > > > > 
> > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
> > > > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 21b04c3..e990c8b 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -190,6 +190,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> > > > > >  	return (max_link_clock * max_lanes * 8) / 10;
> > > > > >  }
> > > > > >  
> > > > > > +static int
> > > > > > +intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp, int dotclk)
> > > > > > +{
> > > > > 
> > > > > I would just
> > > > > 
> > > > > {
> > > > > 	int max_dotclk = dev_priv->max_dotclk_freq;
> > > > > 
> > > > > 	ds_max_dotclk = ...;
> > > > > 	if (ds_dotclk != 0)
> > > > > 		max_dotclk = min(max_dotclk, ds_max_dotclk);
> > > > > 
> > > > > 	return max_dotclk;
> > > > > }
> > > > > 
> > > > > > +	int ds_dotclk;
> > > > > > +	int type;
> > > > > > +
> > > > > > +	ds_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
> > > > > > +						intel_dp->downstream_ports);
> > > > > > +
> > > > > > +	if (ds_dotclk == 0)
> > > > > > +		return dotclk;
> > > > > > +
> > > > > > +	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> > > > > > +	
> > > > > > +	if (type != DP_DS_PORT_TYPE_VGA)
> > > > > > +		return dotclk;
> > > > > 
> > > > > Why isn't drm_dp_downstream_max_clock() handling all of it already?
> > > > > Why are we even checking for !=VGA?
> > > > The routine drm_dp_downstream_max_clock returns the clock rate which can
> > > > be dotclock (VGA only) or TMDS clock (for DVI, HDMI, DP++). Here, we
> > > > need to have a check for this as we are only interested to update VGA
> > > > dotclock value.
> > > 
> > > We should handle it all. Actually I'm not even sure how we're supposed
> > > to deal with the downstream port max TMDS clock since for HDMI that
> > > depends on the bpc, but since this is about a DP->HDMI conversion, I
> > > don't know if we have to take the downstream port max TMDS clock into
> > > account when choosing the bpc over the DP link as well. I suppose that's
> > > possible if the dongle can't change change the bpc, and instead just
> > > passes things through. I think this is one of those places where the
> > > DP spec is way too unclear. But for DP->VGA there is no clock going out
> > > the other end, so it must be just about the limits of the DP input or
> > > the DAC.
> > 
> > I guess we should defensively assume that the tmds clock limit is both for
> > the input and the output signal, worst case, for the dp->hdmi dongle?
> > Except when it's a passive level-shifter only one ofc.
> > -Daniel
> So, we should respect dongles both tmds and bpc values. I thought that
> tmds clock checks was already covered by Ville's DP dual mode patch
> series?

That doesn't deal with active DP->HDMI converters, which is what this
DPCD stuff will have to deal with.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel