| Message ID | 1461979695-27113-5-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 29, 2016 at 06:28:14PM -0700, Manasi Navare wrote: > From: Jim Bride <jim.bride@linux.intel.com> > > For DP compliance we need to be able to control the output color > type for the pipe associated with the DP port. To do this we rely > on the intel_dp_test_force_bpc debugfs file and the associated value > stored in struct intel_dp. If the debugfs file has a non-zero value > and we're on a display port connector, then we use the value from > debugfs to calculate the bpp for the pipe. For cases where we are > not on DP or there has not been an overridden value then we behave > as normal. > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 5ffccf6..1618d36 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc, > > /* Clamp display bpp to EDID value */ > for_each_connector_in_state(state, connector, connector_state, i) { > + int type = 0; > + > + if (connector_state->best_encoder) { > + struct intel_encoder *ienc; > + > + ienc = to_intel_encoder(connector_state->best_encoder); > + type = ienc->type; > + } > + > if (connector_state->crtc != &crtc->base) > continue; > > - connected_sink_compute_bpp(to_intel_connector(connector), > - pipe_config); > + /* For DP compliance we need to ensure that we can override > + * the computed bpp for the pipe. > + */ > + if (type == INTEL_OUTPUT_DISPLAYPORT) { > + struct intel_dp *intel_dp = > + enc_to_intel_dp(connector_state->best_encoder); > + > + if (intel_dp && > + (intel_dp->compliance_force_bpc != 0)) { > + pipe_config->pipe_bpp = > + intel_dp->compliance_force_bpc*3; > + DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n", > + pipe_config->pipe_bpp); I missed this before, but you should probably take the 'JMB' out of the debug message above. Jim > + } else { > + connected_sink_compute_bpp(to_intel_connector(connector), > + pipe_config); > + } > + } else { > + connected_sink_compute_bpp(to_intel_connector(connector), > + pipe_config); > + } > } > > return bpp; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e23eed7..10eaff8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -865,6 +865,7 @@ struct intel_dp { > unsigned long compliance_test_type; > unsigned long compliance_test_data; > bool compliance_test_active; > + unsigned long compliance_force_bpc; /* 0 for default or bpc to use */ > }; > > struct intel_digital_port { > -- > 1.9.1
On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote: > From: Jim Bride <jim.bride@linux.intel.com> > > For DP compliance we need to be able to control the output color > type for the pipe associated with the DP port. To do this we rely > on the intel_dp_test_force_bpc debugfs file and the associated value > stored in struct intel_dp. If the debugfs file has a non-zero value > and we're on a display port connector, then we use the value from > debugfs to calculate the bpp for the pipe. For cases where we are > not on DP or there has not been an overridden value then we behave > as normal. > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 5ffccf6..1618d36 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc, > > /* Clamp display bpp to EDID value */ > for_each_connector_in_state(state, connector, connector_state, i) { > + int type = 0; > + > + if (connector_state->best_encoder) { > + struct intel_encoder *ienc; > + > + ienc = to_intel_encoder(connector_state- > >best_encoder); > + type = ienc->type; > + } > + > if (connector_state->crtc != &crtc->base) > continue; > > - connected_sink_compute_bpp(to_intel_connector(connector), > - pipe_config); > + /* For DP compliance we need to ensure that we can override > + * the computed bpp for the pipe. > + */ > + if (type == INTEL_OUTPUT_DISPLAYPORT) { > + struct intel_dp *intel_dp = > + enc_to_intel_dp(connector_state- > >best_encoder); > + > + if (intel_dp && > + (intel_dp->compliance_force_bpc != 0)) { > + pipe_config->pipe_bpp = > + intel_dp->compliance_force_bpc*3; > + DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n", > + pipe_config->pipe_bpp); > + } else { > + connected_sink_compute_bpp(to_intel_connector > (connector), > + pipe_config); This kind of thing should be done in the encoder compute_config hook. > + } > + } else { > + connected_sink_compute_bpp(to_intel_connector(connect > or), > + pipe_config); > + } > } > > return bpp; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index e23eed7..10eaff8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -865,6 +865,7 @@ struct intel_dp { > unsigned long compliance_test_type; > unsigned long compliance_test_data; > bool compliance_test_active; > + unsigned long compliance_force_bpc; /* 0 for default or bpc to use */ There's no code for setting compliance_test_active anywhere. The commit message mentions debugfs, but I didn't see anything related in the patch. Ander
On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote: > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote: > > From: Jim Bride <jim.bride@linux.intel.com> > > > > For DP compliance we need to be able to control the output color > > type for the pipe associated with the DP port. To do this we rely > > on the intel_dp_test_force_bpc debugfs file and the associated value > > stored in struct intel_dp. If the debugfs file has a non-zero value > > and we're on a display port connector, then we use the value from > > debugfs to calculate the bpp for the pipe. For cases where we are > > not on DP or there has not been an overridden value then we behave > > as normal. > > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++-- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 2 files changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 5ffccf6..1618d36 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc, > > > > /* Clamp display bpp to EDID value */ > > for_each_connector_in_state(state, connector, connector_state, i) { > > + int type = 0; > > + > > + if (connector_state->best_encoder) { > > + struct intel_encoder *ienc; > > + > > + ienc = to_intel_encoder(connector_state- > > >best_encoder); > > + type = ienc->type; > > + } > > + > > if (connector_state->crtc != &crtc->base) > > continue; > > > > - connected_sink_compute_bpp(to_intel_connector(connector), > > - pipe_config); > > + /* For DP compliance we need to ensure that we can override > > + * the computed bpp for the pipe. > > + */ > > + if (type == INTEL_OUTPUT_DISPLAYPORT) { > > + struct intel_dp *intel_dp = > > + enc_to_intel_dp(connector_state- > > >best_encoder); > > + > > + if (intel_dp && > > + (intel_dp->compliance_force_bpc != 0)) { > > + pipe_config->pipe_bpp = > > + intel_dp->compliance_force_bpc*3; > > + DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n", > > + pipe_config->pipe_bpp); > > + } else { > > + connected_sink_compute_bpp(to_intel_connector > > (connector), > > + pipe_config); > > This kind of thing should be done in the encoder compute_config hook. Even though it's really not specific to an individual encoder configuration? This seems to be the central place where we're ensuring that we have a sane value for bpp relative to the display, and thus a good place to set this override to make compliance happy (which needs a specific bpc value for some test cases rather than one that is deemed sane relative to the sink's capabilities. > > > + } > > + } else { > > + connected_sink_compute_bpp(to_intel_connector(connect > > or), > > + pipe_config); > > + } > > } > > > > return bpp; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index e23eed7..10eaff8 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -865,6 +865,7 @@ struct intel_dp { > > unsigned long compliance_test_type; > > unsigned long compliance_test_data; > > bool compliance_test_active; > > + unsigned long compliance_force_bpc; /* 0 for default or bpc to use */ > > There's no code for setting compliance_test_active anywhere. The commit message > mentions debugfs, but I didn't see anything related in the patch. It appears that Manasi forgot to include one of the patches I had sent her. The debugfs support code was in a separate patch. Jim > > > Ander > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2016-05-23 at 10:42 -0700, Jim Bride wrote: > On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote: > > > > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote: > > > > > > From: Jim Bride <jim.bride@linux.intel.com> > > > > > > For DP compliance we need to be able to control the output color > > > type for the pipe associated with the DP port. To do this we rely > > > on the intel_dp_test_force_bpc debugfs file and the associated value > > > stored in struct intel_dp. If the debugfs file has a non-zero value > > > and we're on a display port connector, then we use the value from > > > debugfs to calculate the bpp for the pipe. For cases where we are > > > not on DP or there has not been an overridden value then we behave > > > as normal. > > > > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++- > > > - > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > 2 files changed, 31 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 5ffccf6..1618d36 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc > > > *crtc, > > > > > > /* Clamp display bpp to EDID value */ > > > for_each_connector_in_state(state, connector, connector_state, i) > > > { > > > + int type = 0; > > > + > > > + if (connector_state->best_encoder) { > > > + struct intel_encoder *ienc; > > > + > > > + ienc = to_intel_encoder(connector_state- > > > > > > > > best_encoder); > > > + type = ienc->type; > > > + } > > > + > > > if (connector_state->crtc != &crtc->base) > > > continue; > > > > > > - connected_sink_compute_bpp(to_intel_connector(connector), > > > - pipe_config); > > > + /* For DP compliance we need to ensure that we can > > > override > > > + * the computed bpp for the pipe. > > > + */ > > > + if (type == INTEL_OUTPUT_DISPLAYPORT) { > > > + struct intel_dp *intel_dp = > > > + enc_to_intel_dp(connector_state- > > > > > > > > best_encoder); > > > + > > > + if (intel_dp && > > > + (intel_dp->compliance_force_bpc != 0)) { > > > + pipe_config->pipe_bpp = > > > + intel_dp->compliance_force_bpc*3; > > > + DRM_DEBUG_KMS("JMB Setting pipe_bpp to > > > %d\n", > > > + pipe_config->pipe_bpp); > > > + } else { > > > + connected_sink_compute_bpp(to_intel_conne > > > ctor > > > (connector), > > > + pipe_config); > > This kind of thing should be done in the encoder compute_config hook. > Even though it's really not specific to an individual encoder configuration? > This seems to be the central place where we're ensuring that we have a sane > value for bpp relative to the display, and thus a good place to set this > override to make compliance happy (which needs a specific bpc value for some > test cases rather than one that is deemed sane relative to the sink's > capabilities. Well, this code path is only reached when the DisplayPort associated with a given encoder is in the middle of compliance testing. I'd say that's very encoder specific. The bpp computation happens in two phases. Here a baseline is computed, considering what is generally supported by the hardware. The encoders are allowed to override that value. You can look at HDMI for an example: it may require a bpp override since HDMI doesn't supports 10 bpc, only 8 or 12. You can find similar code also in LVDS and even DP. Unfortunately, there is very little documentation of what the hooks are supposed to do. But for the question at hand, yes, it should really be in intel_dp_compute_config(). Ander > > > > > > > > > > > + } > > > + } else { > > > + connected_sink_compute_bpp(to_intel_connector(con > > > nect > > > or), > > > + pipe_config); > > > + } > > > } > > > > > > return bpp; > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index e23eed7..10eaff8 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -865,6 +865,7 @@ struct intel_dp { > > > unsigned long compliance_test_type; > > > unsigned long compliance_test_data; > > > bool compliance_test_active; > > > + unsigned long compliance_force_bpc; /* 0 for default or bpc to > > > use */ > > There's no code for setting compliance_test_active anywhere. The commit > > message > > mentions debugfs, but I didn't see anything related in the patch. > It appears that Manasi forgot to include one of the patches I had sent her. > The debugfs support code was in a separate patch. > > Jim > > > > > > > > > Ander > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 23, 2016 at 10:42:41AM -0700, Jim Bride wrote: > On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote: > > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote: > > > From: Jim Bride <jim.bride@linux.intel.com> > > > > > > For DP compliance we need to be able to control the output color > > > type for the pipe associated with the DP port. To do this we rely > > > on the intel_dp_test_force_bpc debugfs file and the associated value > > > stored in struct intel_dp. If the debugfs file has a non-zero value > > > and we're on a display port connector, then we use the value from > > > debugfs to calculate the bpp for the pipe. For cases where we are > > > not on DP or there has not been an overridden value then we behave > > > as normal. > > > > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++-- > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > 2 files changed, 31 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 5ffccf6..1618d36 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc, > > > > > > /* Clamp display bpp to EDID value */ > > > for_each_connector_in_state(state, connector, connector_state, i) { > > > + int type = 0; > > > + > > > + if (connector_state->best_encoder) { > > > + struct intel_encoder *ienc; > > > + > > > + ienc = to_intel_encoder(connector_state- > > > >best_encoder); > > > + type = ienc->type; > > > + } > > > + > > > if (connector_state->crtc != &crtc->base) > > > continue; > > > > > > - connected_sink_compute_bpp(to_intel_connector(connector), > > > - pipe_config); > > > + /* For DP compliance we need to ensure that we can override > > > + * the computed bpp for the pipe. > > > + */ > > > + if (type == INTEL_OUTPUT_DISPLAYPORT) { > > > + struct intel_dp *intel_dp = > > > + enc_to_intel_dp(connector_state- > > > >best_encoder); > > > + > > > + if (intel_dp && > > > + (intel_dp->compliance_force_bpc != 0)) { > > > + pipe_config->pipe_bpp = > > > + intel_dp->compliance_force_bpc*3; > > > + DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n", > > > + pipe_config->pipe_bpp); > > > + } else { > > > + connected_sink_compute_bpp(to_intel_connector > > > (connector), > > > + pipe_config); > > > > This kind of thing should be done in the encoder compute_config hook. > > Even though it's really not specific to an individual encoder configuration? > This seems to be the central place where we're ensuring that we have a sane > value for bpp relative to the display, and thus a good place to set this > override to make compliance happy (which needs a specific bpc value for some > test cases rather than one that is deemed sane relative to the sink's > capabilities. > > > > > > + } > > > + } else { > > > + connected_sink_compute_bpp(to_intel_connector(connect > > > or), > > > + pipe_config); > > > + } > > > } > > > > > > return bpp; > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index e23eed7..10eaff8 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -865,6 +865,7 @@ struct intel_dp { > > > unsigned long compliance_test_type; > > > unsigned long compliance_test_data; > > > bool compliance_test_active; > > > + unsigned long compliance_force_bpc; /* 0 for default or bpc to use */ > > > > There's no code for setting compliance_test_active anywhere. The commit message > > mentions debugfs, but I didn't see anything related in the patch. The compliance_test_active is also being set as part of the compliance test handlers for edid and video pattern generation. > > It appears that Manasi forgot to include one of the patches I had sent her. > The debugfs support code was in a separate patch. > > Jim We do not need the debugfs node to set the bpc value. Instead this bpc value in intel_dp structure is being set as a part of video_pattern_autotes. Instead of userspace setting this value through debugfs node, the kernel is setting this directly. I will update the commit message to remove the debugfs node setting. Manasi > > > > > > > Ander > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, May 24, 2016 at 08:45:45AM +0300, Ander Conselvan De Oliveira wrote: > On Mon, 2016-05-23 at 10:42 -0700, Jim Bride wrote: > > On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote: > > > > > > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote: > > > > > > > > From: Jim Bride <jim.bride@linux.intel.com> > > > > > > > > For DP compliance we need to be able to control the output color > > > > type for the pipe associated with the DP port. To do this we rely > > > > on the intel_dp_test_force_bpc debugfs file and the associated value > > > > stored in struct intel_dp. If the debugfs file has a non-zero value > > > > and we're on a display port connector, then we use the value from > > > > debugfs to calculate the bpp for the pipe. For cases where we are > > > > not on DP or there has not been an overridden value then we behave > > > > as normal. > > > > > > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++- > > > > - > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > 2 files changed, 31 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 5ffccf6..1618d36 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc > > > > *crtc, > > > > > > > > /* Clamp display bpp to EDID value */ > > > > for_each_connector_in_state(state, connector, connector_state, i) > > > > { > > > > + int type = 0; > > > > + > > > > + if (connector_state->best_encoder) { > > > > + struct intel_encoder *ienc; > > > > + > > > > + ienc = to_intel_encoder(connector_state- > > > > > > > > > > best_encoder); > > > > + type = ienc->type; > > > > + } > > > > + > > > > if (connector_state->crtc != &crtc->base) > > > > continue; > > > > > > > > - connected_sink_compute_bpp(to_intel_connector(connector), > > > > - pipe_config); > > > > + /* For DP compliance we need to ensure that we can > > > > override > > > > + * the computed bpp for the pipe. > > > > + */ > > > > + if (type == INTEL_OUTPUT_DISPLAYPORT) { > > > > + struct intel_dp *intel_dp = > > > > + enc_to_intel_dp(connector_state- > > > > > > > > > > best_encoder); > > > > + > > > > + if (intel_dp && > > > > + (intel_dp->compliance_force_bpc != 0)) { > > > > + pipe_config->pipe_bpp = > > > > + intel_dp->compliance_force_bpc*3; > > > > + DRM_DEBUG_KMS("JMB Setting pipe_bpp to > > > > %d\n", > > > > + pipe_config->pipe_bpp); > > > > + } else { > > > > + connected_sink_compute_bpp(to_intel_conne > > > > ctor > > > > (connector), > > > > + pipe_config); > > > This kind of thing should be done in the encoder compute_config hook. > > Even though it's really not specific to an individual encoder configuration? > > This seems to be the central place where we're ensuring that we have a sane > > value for bpp relative to the display, and thus a good place to set this > > override to make compliance happy (which needs a specific bpc value for some > > test cases rather than one that is deemed sane relative to the sink's > > capabilities. > > Well, this code path is only reached when the DisplayPort associated with a > given encoder is in the middle of compliance testing. I'd say that's very > encoder specific. > > The bpp computation happens in two phases. Here a baseline is computed, > considering what is generally supported by the hardware. The encoders are > allowed to override that value. You can look at HDMI for an example: it may > require a bpp override since HDMI doesn't supports 10 bpc, only 8 or 12. You can > find similar code also in LVDS and even DP. > > Unfortunately, there is very little documentation of what the hooks are supposed > to do. But for the question at hand, yes, it should really be in > intel_dp_compute_config(). > > Ander Inside of intel_dp_compute_config(), do we need to change the pipe_config->pipe_bpp to use intel_dp->compliance_force_bpc in case of CTS or should we just modify the local bpp value to use the force_bpc? Manasi > > > > > > > > > > > > > > > > > + } > > > > + } else { > > > > + connected_sink_compute_bpp(to_intel_connector(con > > > > nect > > > > or), > > > > + pipe_config); > > > > + } > > > > } > > > > > > > > return bpp; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index e23eed7..10eaff8 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -865,6 +865,7 @@ struct intel_dp { > > > > unsigned long compliance_test_type; > > > > unsigned long compliance_test_data; > > > > bool compliance_test_active; > > > > + unsigned long compliance_force_bpc; /* 0 for default or bpc to > > > > use */ > > > There's no code for setting compliance_test_active anywhere. The commit > > > message > > > mentions debugfs, but I didn't see anything related in the patch. > > It appears that Manasi forgot to include one of the patches I had sent her. > > The debugfs support code was in a separate patch. > > > > Jim > > > > > > > > > > > > > > Ander > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2016-05-25 at 17:42 -0700, Manasi Navare wrote: > On Tue, May 24, 2016 at 08:45:45AM +0300, Ander Conselvan De Oliveira wrote: > > > > On Mon, 2016-05-23 at 10:42 -0700, Jim Bride wrote: > > > > > > On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira > > > wrote: > > > > > > > > > > > > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote: > > > > > > > > > > > > > > > From: Jim Bride <jim.bride@linux.intel.com> > > > > > > > > > > For DP compliance we need to be able to control the output color > > > > > type for the pipe associated with the DP port. To do this we rely > > > > > on the intel_dp_test_force_bpc debugfs file and the associated value > > > > > stored in struct intel_dp. If the debugfs file has a non-zero value > > > > > and we're on a display port connector, then we use the value from > > > > > debugfs to calculate the bpp for the pipe. For cases where we are > > > > > not on DP or there has not been an overridden value then we behave > > > > > as normal. > > > > > > > > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 32 > > > > > ++++++++++++++++++++++++++++++- > > > > > - > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > 2 files changed, 31 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index 5ffccf6..1618d36 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc > > > > > *crtc, > > > > > > > > > > /* Clamp display bpp to EDID value */ > > > > > for_each_connector_in_state(state, connector, > > > > > connector_state, i) > > > > > { > > > > > + int type = 0; > > > > > + > > > > > + if (connector_state->best_encoder) { > > > > > + struct intel_encoder *ienc; > > > > > + > > > > > + ienc = to_intel_encoder(connector_state- > > > > > > > > > > > > > > > > > > best_encoder); > > > > > + type = ienc->type; > > > > > + } > > > > > + > > > > > if (connector_state->crtc != &crtc->base) > > > > > continue; > > > > > > > > > > - connected_sink_compute_bpp(to_intel_connector(connect > > > > > or), > > > > > - pipe_config); > > > > > + /* For DP compliance we need to ensure that we can > > > > > override > > > > > + * the computed bpp for the pipe. > > > > > + */ > > > > > + if (type == INTEL_OUTPUT_DISPLAYPORT) { > > > > > + struct intel_dp *intel_dp = > > > > > + enc_to_intel_dp(connector_state- > > > > > > > > > > > > > > > > > > best_encoder); > > > > > + > > > > > + if (intel_dp && > > > > > + (intel_dp->compliance_force_bpc != 0)) { > > > > > + pipe_config->pipe_bpp = > > > > > + intel_dp- > > > > > >compliance_force_bpc*3; > > > > > + DRM_DEBUG_KMS("JMB Setting pipe_bpp > > > > > to > > > > > %d\n", > > > > > + pipe_config->pipe_bpp); > > > > > + } else { > > > > > + connected_sink_compute_bpp(to_intel_c > > > > > onne > > > > > ctor > > > > > (connector), > > > > > + pipe_config); > > > > This kind of thing should be done in the encoder compute_config hook. > > > Even though it's really not specific to an individual encoder > > > configuration? > > > This seems to be the central place where we're ensuring that we have a > > > sane > > > value for bpp relative to the display, and thus a good place to set this > > > override to make compliance happy (which needs a specific bpc value for > > > some > > > test cases rather than one that is deemed sane relative to the sink's > > > capabilities. > > Well, this code path is only reached when the DisplayPort associated with a > > given encoder is in the middle of compliance testing. I'd say that's very > > encoder specific. > > > > The bpp computation happens in two phases. Here a baseline is computed, > > considering what is generally supported by the hardware. The encoders are > > allowed to override that value. You can look at HDMI for an example: it may > > require a bpp override since HDMI doesn't supports 10 bpc, only 8 or 12. You > > can > > find similar code also in LVDS and even DP. > > > > Unfortunately, there is very little documentation of what the hooks are > > supposed > > to do. But for the question at hand, yes, it should really be in > > intel_dp_compute_config(). > > > > Ander > Inside of intel_dp_compute_config(), do we need to change the pipe_config- > >pipe_bpp > to use intel_dp->compliance_force_bpc in case of CTS or should we just modify > the local > bpp value to use the force_bpc? I think is enough to change the bpp variable before the loop that decides the link rate and lane count, similarly to what is done in the eDP case when a bpp value is read form the VBT. Ander > > Manasi > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + } > > > > > + } else { > > > > > + connected_sink_compute_bpp(to_intel_connector > > > > > (con > > > > > nect > > > > > or), > > > > > + pipe_config); > > > > > + } > > > > > } > > > > > > > > > > return bpp; > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > > index e23eed7..10eaff8 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -865,6 +865,7 @@ struct intel_dp { > > > > > unsigned long compliance_test_type; > > > > > unsigned long compliance_test_data; > > > > > bool compliance_test_active; > > > > > + unsigned long compliance_force_bpc; /* 0 for default or bpc > > > > > to > > > > > use */ > > > > There's no code for setting compliance_test_active anywhere. The commit > > > > message > > > > mentions debugfs, but I didn't see anything related in the patch. > > > It appears that Manasi forgot to include one of the patches I had sent > > > her. > > > The debugfs support code was in a separate patch. > > > > > > Jim > > > > > > > > > > > > > > > > > > > > > > > Ander > > > > _______________________________________________ > > > > 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_display.c b/drivers/gpu/drm/i915/intel_display.c index 5ffccf6..1618d36 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc, /* Clamp display bpp to EDID value */ for_each_connector_in_state(state, connector, connector_state, i) { + int type = 0; + + if (connector_state->best_encoder) { + struct intel_encoder *ienc; + + ienc = to_intel_encoder(connector_state->best_encoder); + type = ienc->type; + } + if (connector_state->crtc != &crtc->base) continue; - connected_sink_compute_bpp(to_intel_connector(connector), - pipe_config); + /* For DP compliance we need to ensure that we can override + * the computed bpp for the pipe. + */ + if (type == INTEL_OUTPUT_DISPLAYPORT) { + struct intel_dp *intel_dp = + enc_to_intel_dp(connector_state->best_encoder); + + if (intel_dp && + (intel_dp->compliance_force_bpc != 0)) { + pipe_config->pipe_bpp = + intel_dp->compliance_force_bpc*3; + DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n", + pipe_config->pipe_bpp); + } else { + connected_sink_compute_bpp(to_intel_connector(connector), + pipe_config); + } + } else { + connected_sink_compute_bpp(to_intel_connector(connector), + pipe_config); + } } return bpp; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e23eed7..10eaff8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -865,6 +865,7 @@ struct intel_dp { unsigned long compliance_test_type; unsigned long compliance_test_data; bool compliance_test_active; + unsigned long compliance_force_bpc; /* 0 for default or bpc to use */ }; struct intel_digital_port {