On Fri, 17 Jan 2020, Jani Nikula wrote:
>On Fri, 17 Jan 2020, Lee Shawn C <shawn.c.lee@xxxxxxxxx> wrote:
>> While mode setting, driver would calculate mode rate based on
>> resolution and bpp. And choose the best bpp that did not exceed DP
>> bandwidtd.
>>
>> But LSPCON had more restriction due to it convert DP to HDMI.
>> Driver should respect HDMI's bandwidth limitation if LSPCON was
>> active. This change would ignore the bpp when its required output
>> bandwidth already over HDMI 2.0 or 1.4 spec.
>>
>> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx>
>> Cc: Sam McNally <sammc@xxxxxxxxxx>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/display/intel_dp.c | 45 +++++++++++++++++++++
>> drivers/gpu/drm/i915/display/intel_lspcon.c | 5 +++
>> drivers/gpu/drm/i915/display/intel_lspcon.h | 1 +
>> 3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index c7424e2a04a3..c27d3e7ac219 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -1976,6 +1976,47 @@ static int intel_dp_output_bpp(const struct intel_crtc_state *crtc_state, int bp
>> return bpp;
>> }
>>
>> +static bool
>> +intel_dp_lspcon_exceed_bandwidth_limitation(struct intel_dp *intel_dp,
>> + struct intel_crtc_state *pipe_config,
>> + int bpp)
>> +{
>> + struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>> + struct intel_connector *connector = intel_dp->attached_connector;
>> + const struct drm_display_info *info = &connector->base.display_info;
>> + enum drm_lspcon_mode lspcon_current_mode = lspcon_get_mode(lspcon);
>> + const int pcon_mode_max_tmds_clock = 600000;
>> + const int ls_mode_max_tmds_clock = 340000;
>> + int mode_rate, max_tmds_clock = pcon_mode_max_tmds_clock;
>> +
>> + if (lspcon->active) {
>> + switch (bpp) {
>> + case 36:
>> + mode_rate = pipe_config->hw.adjusted_mode.crtc_clock * 3 / 2;
>> + break;
>> + case 30:
>> + mode_rate = pipe_config->hw.adjusted_mode.crtc_clock * 5 / 4;
>> + break;
>> + case 24:
>> + default:
>> + mode_rate = pipe_config->hw.adjusted_mode.crtc_clock;
>> + break;
>> + }
>> +
>> + if (lspcon_current_mode == DRM_LSPCON_MODE_LS)
>> + max_tmds_clock = ls_mode_max_tmds_clock;
>> +
>> + if (info->max_tmds_clock)
>> + max_tmds_clock = min(max_tmds_clock,
>> + info->max_tmds_clock);
>> +
>> + if (mode_rate > max_tmds_clock)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>
>Instead of this, please add a simple intel_lspcon.c function:
>
> int lspcon_max_rate(struct intel_lspcon *lspcon);
>
>that returns the max rate. Everything else can then be done in intel_dp.c around this. The function gets simplified enough that you can throw out the const ints and use the values directly.
>
Thanks for comment! I will rename and move it to intel_lspcon.c.
>> /* Optimize link config in order: max bpp, min clock, min lanes */
>> static int intel_dp_compute_link_config_wide(struct intel_dp
>> *intel_dp, @@ -1989,6 +2030,10 @@
>> intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>> for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
>> int output_bpp = intel_dp_output_bpp(pipe_config, bpp);
>>
>> + /* Bypass th bpp if require bandwidth over HDMI spec when LSPCON active */
>> + if (intel_dp_lspcon_exceed_bandwidth_limitation(intel_dp, pipe_config, output_bpp))
>> + continue;
>> +
>
>The placing sticks out like a sore thumb. I think we need to filter out the modes already in intel_dp_mode_valid().
>This isn't all that different from intel_dp_downstream_max_dotclock() is it?
Yes, what you said is right. intel_dp_mode_valid() did all the thing based on source DP output capability.
And intel_dp_downstream_max_dotclock() report LSPCON's DP RX bandwidth.
We have to consider LSPCON's HDMI TX limitation as well to avoid display output data over HDMI's spec.
>
>> mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>> output_bpp);
>
>But I guess since the mode valid limit is for 8 bpc, you'll also need a check here? Maybe Ville has better ideas.
>
>Here it would be something like:
>
> if (lspcon->active && mode_rate > lspcon_max_rate(lscon))
> continue;
Let's wait for more comments from Ville.
I will follow the suggestion to update v2 patch later.
Best regards,
Shawn
>
>BR,
>Jani.
>
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c
>> b/drivers/gpu/drm/i915/display/intel_lspcon.c
>> index d807c5648c87..6952c5028fdf 100644
>> --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
>> @@ -550,6 +550,11 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon)
>> lspcon_wait_mode(lspcon, DRM_LSPCON_MODE_PCON); }
>>
>> +int lspcon_get_mode(struct intel_lspcon *lspcon) {
>> + return lspcon_get_current_mode(lspcon); }
>> +
>> bool lspcon_init(struct intel_digital_port *intel_dig_port) {
>> struct intel_dp *dp = &intel_dig_port->dp; diff --git
>> a/drivers/gpu/drm/i915/display/intel_lspcon.h
>> b/drivers/gpu/drm/i915/display/intel_lspcon.h
>> index 37cfddf8a9c5..5ce9daef9708 100644
>> --- a/drivers/gpu/drm/i915/display/intel_lspcon.h
>> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h
>> @@ -18,6 +18,7 @@ struct intel_lspcon; bool lspcon_init(struct
>> intel_digital_port *intel_dig_port); void lspcon_resume(struct
>> intel_lspcon *lspcon); void lspcon_wait_pcon_mode(struct intel_lspcon
>> *lspcon);
>> +int lspcon_get_mode(struct intel_lspcon *lspcon);
>> void lspcon_write_infoframe(struct intel_encoder *encoder,
>> const struct intel_crtc_state *crtc_state,
>> unsigned int type,
>
>--
>Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
