Re: [PATCH 2/2] drm/i915/dp: Validate mode against max. link data rate for DP MST

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

On Mon, Nov 14, 2016 at 09:35:30PM +0000, Pandiyan, Dhinakaran wrote:
> On Thu, 2016-11-10 at 15:32 -0800, Manasi Navare wrote:
> > On Wed, Nov 09, 2016 at 09:32:30PM -0800, Dhinakaran Pandiyan wrote:
> > > Not validating the the mode rate against link rate results not pruning
> > > invalid modes. For e.g, HBR2 5.4 Gpbs 2 lane configuration does not
> > > support 4k @ 60Hz. But, we do not reject this mode currently.
> > > 
> > > So, make use of the helpers in intel_dp in validate mode rates against
> > > max. data rate of a configuration.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c     |  4 ++--
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 12 +++++++++++-
> > >  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
> > >  3 files changed, 15 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 7a9e122..7a73e43 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -161,14 +161,14 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
> > >  	return min(source_max, sink_max);
> > >  }
> > >  
> > > -static int
> > > +int
> > >  intel_dp_link_required(int pixel_clock, int bpp)
> > >  {
> > >  	/* pixel_clock is in kHz, divide bpp by 8 to return the value in kBps*/
> > >  	return (pixel_clock * bpp + 7) / 8;
> > >  }
> > >  
> > > -static int
> > > +int
> > >  intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> > >  {
> > >  	/* max_link_clock is the link symbol clock (LS_Clk) in kHz and not the
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 3ffbd69..38d2ce0 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -335,7 +335,17 @@ static enum drm_mode_status
> > >  intel_dp_mst_mode_valid(struct drm_connector *connector,
> > >  			struct drm_display_mode *mode)
> > >  {
> > > +	struct intel_connector *intel_connector = to_intel_connector(connector);
> > > +	struct intel_dp *intel_dp = intel_connector->mst_port;
> > >  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> > > +	int link_clock = intel_dp_max_link_rate(intel_dp);
> > > +	int lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
> > > +	int bpp = 24; /* MST uses fixed bpp */
> > > +	int mode_rate;
> > > +	int link_max_data_rate;
> > 
> > In the SST equivalent mode_valid function, this variable is named as
> > max_rate, I think you should name it as max_rate as well for consistency.
> > Other than that this looks good, we definitely need this for mode validation
> > at an early stage.
> > 
> > Regards
> > Manasi
> > 
> 
> Well, one of the goals of Patch 1/2 is to reduce ambiguity that has led
> to some confusing hacks. I prefer clarity over consistency and anyway
> this variable is local to this function :)

I'm with Manasi on this one. Consistency wins. We have far too many
things we call by many names, and that always causes confusion. If you
think the name isn't good enough, then you should rename it across the
board.

> 
> -DK
> 
> > > +
> > > +	link_max_data_rate = intel_dp_max_data_rate(link_clock, lane_count);
> > > +	mode_rate = intel_dp_link_required(mode->clock, bpp);
> > >  
> > >  	/* TODO - validate mode against available PBN for link */
> > >  	if (mode->clock < 10000)
> > > @@ -344,7 +354,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
> > >  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > >  		return MODE_H_ILLEGAL;
> > >  
> > > -	if (mode->clock > max_dotclk)
> > > +	if (mode_rate > link_max_data_rate || mode->clock > max_dotclk)
> > >  		return MODE_CLOCK_HIGH;
> > >  
> > >  	return MODE_OK;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index c2f3863..313419d 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1471,6 +1471,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > >  bool __intel_dp_read_desc(struct intel_dp *intel_dp,
> > >  			  struct intel_dp_desc *desc);
> > >  bool intel_dp_read_desc(struct intel_dp *intel_dp);
> > > +int intel_dp_link_required(int pixel_clock, int bpp);
> > > +int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > >  
> > >  /* intel_dp_aux_backlight.c */
> > >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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