Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx