| Message ID | 1389296830-3480-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Yay, I'm really happy that after fbc testcases for psr are now also shaping up. Some small stuff below. -Daniel On Thu, Jan 9, 2014 at 8:47 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote: > This debugfs interface will allow intel-gpu-tools test case > to verify if screen has been updated properly on cases like PSR. > > Since the current target is PSR we will provide only the CRC check > for eDP panels. We can latter extend it to all available DP panels. Sob-line? Also I think it'd be nice to have a very basic test just to exercise this code. Probably simplest would be to extend Damien's basic crc testcase: - If there's no edp connector, then skip it. - Skip when the debugfs file isn't there. - Skip if the panel doesn't support CRC (see below). - Display a black screen, check that the crc is stable. - Switch to a white screen, check that the crc is different, but again stable. > --- > drivers/gpu/drm/i915/i915_debugfs.c | 24 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 31 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 9 +++++++++ > include/drm/drm_dp_helper.h | 10 ++++++++++ > 4 files changed, 74 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 75a489e..0facff1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1876,6 +1876,29 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > return 0; > } > > +static int i915_sink_crc(struct seq_file *m, void *data) > +{ > + struct drm_info_node *node = m->private; > + struct drm_device *dev = node->minor->dev; > + struct intel_encoder *encoder; > + struct intel_dp *intel_dp = NULL; > + We need to grab the modeset lock around this loop. > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > + if (encoder->type == INTEL_OUTPUT_EDP) { > + intel_dp = enc_to_intel_dp(&encoder->base); I think we should skip the output if it's not connected to any pipe or if the connector is not in DPMS on type. Also, since this is about the panel I think it's better to loop over connectors. > + > + intel_dp_sink_crc(intel_dp); > + seq_printf(m, "%02hx%02hx%02hx%02hx%02hx%02hx\n", > + intel_dp->sink_crc.r_cr[0], > + intel_dp->sink_crc.r_cr[1], > + intel_dp->sink_crc.g_y[0], > + intel_dp->sink_crc.g_y[1], > + intel_dp->sink_crc.b_cb[0], > + intel_dp->sink_crc.b_cb[1]); Imo it's better to pass an explicit crc array around instead of storing the last CRC in the intel_dp struct. Also, intel_dp_sink_crc should return an error code in case the sink doesn't support CRCs or something else failed. > + } > + return 0; We need some return values here for tests I think: - 0: success, CRC printed with seq_printf. - -EINVAL: The output isn't enabled at all. - -ENOTTY: The panel/output doesn't support CRCs: - Or other error codes for dp aux failed and whatever else can go wrong. > +} > + > static int i915_energy_uJ(struct seq_file *m, void *data) > { > struct drm_info_node *node = m->private; > @@ -3232,6 +3255,7 @@ static const struct drm_info_list i915_debugfs_list[] = { > {"i915_dpio", i915_dpio_info, 0}, > {"i915_llc", i915_llc, 0}, > {"i915_edp_psr_status", i915_edp_psr_status, 0}, > + {"i915_sink_crc", i915_sink_crc, 0}, We need some room to also support eDP2 and DP1, ... in the future maybe. Simplest option would be to add an _eDP1 suffix, a control file like for pipe crcs is imo complete overkill. > {"i915_energy_uJ", i915_energy_uJ, 0}, > {"i915_pc8_status", i915_pc8_status, 0}, > {"i915_power_domain_info", i915_power_domain_info, 0}, > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 7df5085..9933327 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2786,6 +2786,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > struct drm_i915_private *dev_priv = dev->dev_private; > > char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3]; > + u8 buf[1]; > > if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, > sizeof(intel_dp->dpcd)) == 0) > @@ -2810,6 +2811,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > } > } > > + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf, 1); > + intel_dp->sink_crc.supported = buf[0] & DP_TEST_CRC_SUPPORTED; Personally I'd just recheck this in intel_dp_sink_crc instead of caching it to avoid stale values and have simpler control flow. > + > if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & > DP_DWN_STRM_PORT_PRESENT)) > return true; /* native DP sink */ > @@ -2846,6 +2850,33 @@ intel_dp_probe_oui(struct intel_dp *intel_dp) > ironlake_edp_panel_vdd_off(intel_dp, false); > } > > +void intel_dp_sink_crc(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_crtc *intel_crtc = > + to_intel_crtc(intel_dig_port->base.base.crtc); > + > + if (!intel_dp->sink_crc.supported) > + return; > + > + intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, DP_TEST_SINK_START); > + > + /* Wait 2 vblanks to be sure we will have the correct CRC value */ > + intel_wait_for_vblank(dev, intel_crtc->pipe); > + intel_wait_for_vblank(dev, intel_crtc->pipe); > + > + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, > + intel_dp->sink_crc.r_cr, 2); > + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_G_Y, > + intel_dp->sink_crc.g_y, 2); > + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_B_CB, > + intel_dp->sink_crc.b_cb, 2); Iirc you could just read all 6 bytes in one go, dp aux transfers can be up to 16 bytes. > + > + intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, > + ~DP_TEST_SINK_START); Shouldn't we just write a 0 here? > +} > + > static bool > intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) > { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 46aea6c..48533c0 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -462,6 +462,13 @@ struct intel_hdmi { > > #define DP_MAX_DOWNSTREAM_PORTS 0x10 > > +struct sink_crc { > + bool supported; > + u8 r_cr[2]; > + u8 g_y[2]; > + u8 b_cb[2]; > +}; > + > struct intel_dp { > uint32_t output_reg; > uint32_t aux_ch_ctl_reg; > @@ -487,6 +494,7 @@ struct intel_dp { > bool want_panel_vdd; > bool psr_setup_done; > struct intel_connector *attached_connector; > + struct sink_crc sink_crc; > }; > > struct intel_digital_port { > @@ -719,6 +727,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp); > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > void intel_dp_encoder_destroy(struct drm_encoder *encoder); > void intel_dp_check_link_status(struct intel_dp *intel_dp); > +void intel_dp_sink_crc(struct intel_dp *intel_dp); > bool intel_dp_compute_config(struct intel_encoder *encoder, > struct intel_crtc_config *pipe_config); > bool intel_dp_is_edp(struct drm_device *dev, enum port port); > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 1d09050..ba0b90d 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h Better to split this out so non-intel people notice it, just in case. > @@ -279,11 +279,21 @@ > > #define DP_TEST_PATTERN 0x221 > > +#define DP_TEST_CRC_R_CR 0x240 > +#define DP_TEST_CRC_G_Y 0x242 > +#define DP_TEST_CRC_B_CB 0x244 > + > +#define DP_TEST_SINK_MISC 0x246 > +#define DP_TEST_CRC_SUPPORTED (1 << 5) > + > #define DP_TEST_RESPONSE 0x260 > # define DP_TEST_ACK (1 << 0) > # define DP_TEST_NAK (1 << 1) > # define DP_TEST_EDID_CHECKSUM_WRITE (1 << 2) > > +#define DP_TEST_SINK 0x270 > +#define DP_TEST_SINK_START (1 << 0) > + > #define DP_SOURCE_OUI 0x300 > #define DP_SINK_OUI 0x400 > #define DP_BRANCH_OUI 0x500 > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 75a489e..0facff1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1876,6 +1876,29 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) return 0; } +static int i915_sink_crc(struct seq_file *m, void *data) +{ + struct drm_info_node *node = m->private; + struct drm_device *dev = node->minor->dev; + struct intel_encoder *encoder; + struct intel_dp *intel_dp = NULL; + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) + if (encoder->type == INTEL_OUTPUT_EDP) { + intel_dp = enc_to_intel_dp(&encoder->base); + + intel_dp_sink_crc(intel_dp); + seq_printf(m, "%02hx%02hx%02hx%02hx%02hx%02hx\n", + intel_dp->sink_crc.r_cr[0], + intel_dp->sink_crc.r_cr[1], + intel_dp->sink_crc.g_y[0], + intel_dp->sink_crc.g_y[1], + intel_dp->sink_crc.b_cb[0], + intel_dp->sink_crc.b_cb[1]); + } + return 0; +} + static int i915_energy_uJ(struct seq_file *m, void *data) { struct drm_info_node *node = m->private; @@ -3232,6 +3255,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_dpio", i915_dpio_info, 0}, {"i915_llc", i915_llc, 0}, {"i915_edp_psr_status", i915_edp_psr_status, 0}, + {"i915_sink_crc", i915_sink_crc, 0}, {"i915_energy_uJ", i915_energy_uJ, 0}, {"i915_pc8_status", i915_pc8_status, 0}, {"i915_power_domain_info", i915_power_domain_info, 0}, diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7df5085..9933327 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2786,6 +2786,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) struct drm_i915_private *dev_priv = dev->dev_private; char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3]; + u8 buf[1]; if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, sizeof(intel_dp->dpcd)) == 0) @@ -2810,6 +2811,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) } } + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_SINK_MISC, buf, 1); + intel_dp->sink_crc.supported = buf[0] & DP_TEST_CRC_SUPPORTED; + if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT)) return true; /* native DP sink */ @@ -2846,6 +2850,33 @@ intel_dp_probe_oui(struct intel_dp *intel_dp) ironlake_edp_panel_vdd_off(intel_dp, false); } +void intel_dp_sink_crc(struct intel_dp *intel_dp) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct intel_crtc *intel_crtc = + to_intel_crtc(intel_dig_port->base.base.crtc); + + if (!intel_dp->sink_crc.supported) + return; + + intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, DP_TEST_SINK_START); + + /* Wait 2 vblanks to be sure we will have the correct CRC value */ + intel_wait_for_vblank(dev, intel_crtc->pipe); + intel_wait_for_vblank(dev, intel_crtc->pipe); + + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_R_CR, + intel_dp->sink_crc.r_cr, 2); + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_G_Y, + intel_dp->sink_crc.g_y, 2); + intel_dp_aux_native_read_retry(intel_dp, DP_TEST_CRC_B_CB, + intel_dp->sink_crc.b_cb, 2); + + intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, + ~DP_TEST_SINK_START); +} + static bool intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 46aea6c..48533c0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -462,6 +462,13 @@ struct intel_hdmi { #define DP_MAX_DOWNSTREAM_PORTS 0x10 +struct sink_crc { + bool supported; + u8 r_cr[2]; + u8 g_y[2]; + u8 b_cb[2]; +}; + struct intel_dp { uint32_t output_reg; uint32_t aux_ch_ctl_reg; @@ -487,6 +494,7 @@ struct intel_dp { bool want_panel_vdd; bool psr_setup_done; struct intel_connector *attached_connector; + struct sink_crc sink_crc; }; struct intel_digital_port { @@ -719,6 +727,7 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp); void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); void intel_dp_encoder_destroy(struct drm_encoder *encoder); void intel_dp_check_link_status(struct intel_dp *intel_dp); +void intel_dp_sink_crc(struct intel_dp *intel_dp); bool intel_dp_compute_config(struct intel_encoder *encoder, struct intel_crtc_config *pipe_config); bool intel_dp_is_edp(struct drm_device *dev, enum port port); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 1d09050..ba0b90d 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -279,11 +279,21 @@ #define DP_TEST_PATTERN 0x221 +#define DP_TEST_CRC_R_CR 0x240 +#define DP_TEST_CRC_G_Y 0x242 +#define DP_TEST_CRC_B_CB 0x244 + +#define DP_TEST_SINK_MISC 0x246 +#define DP_TEST_CRC_SUPPORTED (1 << 5) + #define DP_TEST_RESPONSE 0x260 # define DP_TEST_ACK (1 << 0) # define DP_TEST_NAK (1 << 1) # define DP_TEST_EDID_CHECKSUM_WRITE (1 << 2) +#define DP_TEST_SINK 0x270 +#define DP_TEST_SINK_START (1 << 0) + #define DP_SOURCE_OUI 0x300 #define DP_SINK_OUI 0x400 #define DP_BRANCH_OUI 0x500