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

2016-11-15 Thread Pandiyan, Dhinakaran 
On Tue, 2016-11-15 at 20:59 +0200, Ville Syrjälä wrote:
> On Mon, Nov 14, 2016 at 09:35:30PM +, 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 
> > > > ---
> > > >  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.
> 

Alright then, I don't want to bikeshed the variable naming anymore.
Sending another version.


> > 
> > -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 < 1)
> > > > @@ -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

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

2016-11-15 Thread Ville Syrjälä 
On Mon, Nov 14, 2016 at 09:35:30PM +, 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 
> > > ---
> > >  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 < 1)
> > > @@ -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
> 
>

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

2016-11-14 Thread Pandiyan, Dhinakaran 
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 
> > ---
> >  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 :)

-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 < 1)
> > @@ -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

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

2016-11-11 Thread Pandiyan, Dhinakaran 
On Fri, 2016-11-11 at 19:41 +0200, Ville Syrjälä 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 
> > ---
> >  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;
> > +
> > +   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 < 1)
> > @@ -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;
> 
> You'll also want to fix intel_dp_mst_compute_config(). Actually that one
> should check against the remaining link bw, but we don't track that
> sufficiently well. Well, actually the topology manager does track it,
> but just not atomically so it's not good enough in its current form.
> 
> But as a first step just checking against the total link bw would be an
> improvement.
> 

I am trying to understand why drm_dp_find_vcpi_slots() returns a value
greater than 64  in the 2 lane HBR2, 4k@60Hz config. Will send out this
patch once that is clear.

-DK


> > 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

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

2016-11-11 Thread Ville Syrjälä 
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 
> ---
>  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;
> +
> + 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 < 1)
> @@ -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;

You'll also want to fix intel_dp_mst_compute_config(). Actually that one
should check against the remaining link bw, but we don't track that
sufficiently well. Well, actually the topology manager does track it,
but just not atomically so it's not good enough in its current form.

But as a first step just checking against the total link bw would be an
improvement.

> 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

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

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

2016-11-10 Thread Manasi Navare 
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 
> ---
>  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

> +
> + 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 < 1)
> @@ -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] [PATCH 2/2] drm/i915/dp: Validate mode against max. link data rate for DP MST

2016-11-09 Thread Dhinakaran Pandiyan 
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 
---
 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;
+
+   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 < 1)
@@ -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