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

[Manasi Navare <manasi.d.navare@intel.com>]

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