| Message ID | 1478755950-2778-2-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > + > + 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
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; > + > + 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; 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
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 <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; > > + > > + 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; > > 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 >
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 :) -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
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
On Tue, 2016-11-15 at 20:59 +0200, Ville Syrjälä wrote: > 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. > 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 < 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 >
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 < 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);
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(-)