Re: [Intel-gfx] [PATCH 7/9] drm/i915/sdvo: Make SDVO deal with HDMI pixel repeat

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 7/9] drm/i915/sdvo: Make SDVO deal with HDMI pixel repeat
Date: Thu, 9 Jul 2020 15:20:08 +0300	[thread overview]
Message-ID: <20200709122008.GZ6112@intel.com> (raw)
In-Reply-To: <20200709114710.GD16776@ideak-desk.fi.intel.com>

On Thu, Jul 09, 2020 at 02:47:10PM +0300, Imre Deak wrote:
> On Wed, Jan 08, 2020 at 08:12:40PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > With SDVO the pipe config pixel_multiplier only concerns itself with the
> > data on the SDVO bus. Any HDMI specific pixel repeat must be handled by
> > the SDVO device itself. To do that simply configure the SDVO pixel
> > replication factor appropriately. We already set up the infoframe PRB
> > values correctly via the infoframe helpers.
> > 
> > There is no cap we can check for this. The spec says that 1X,2X,4X are
> > mandatory, anything else is optional. 1X and 2X are all we need so
> > we should be able to assume they work.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> Shouldn't DRM_MODE_FLAG_DBLCLK be set for the read-out mode in
> intel_sdvo_get_config()?

Apparently we don't do that for HDMI either, and we don't check
it it in intel_pipe_config_compare() either. I guess we could add
the readout+check. 

Although I've been actually tempted to just remove this flag
entirely since it only really should be set to boost the clock
above the magic 25MHz limit. Hmm, now I wonder if that limit
is about the TMDS clock or the dotclock actually. Would need to
read the spec once again. Anyways, the reason why I haven't
deprecated this flag yet is that IIRC some other drivers was
also  using it for other purposes, and I was too lazy to figure
out what's going on there.

> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_sdvo.c | 27 +++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > index a0bbd728aa54..34d5bd750de8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > @@ -943,6 +943,13 @@ static bool intel_sdvo_set_colorimetry(struct intel_sdvo *intel_sdvo,
> >  	return intel_sdvo_set_value(intel_sdvo, SDVO_CMD_SET_COLORIMETRY, &mode, 1);
> >  }
> >  
> > +static bool intel_sdvo_set_pixel_replication(struct intel_sdvo *intel_sdvo,
> > +					     u8 pixel_repeat)
> > +{
> > +	return intel_sdvo_set_value(intel_sdvo, SDVO_CMD_SET_PIXEL_REPLI,
> > +				    &pixel_repeat, 1);
> > +}
> > +
> >  static bool intel_sdvo_set_audio_state(struct intel_sdvo *intel_sdvo,
> >  				       u8 audio_state)
> >  {
> > @@ -1493,6 +1500,9 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
> >  					   SDVO_COLORIMETRY_RGB220 :
> >  					   SDVO_COLORIMETRY_RGB256);
> >  		intel_sdvo_set_avi_infoframe(intel_sdvo, crtc_state);
> > +		intel_sdvo_set_pixel_replication(intel_sdvo,
> > +						 !!(adjusted_mode->flags &
> > +						    DRM_MODE_FLAG_DBLCLK));
> >  	} else
> >  		intel_sdvo_set_encode(intel_sdvo, SDVO_ENCODE_DVI);
> >  
> > @@ -1839,17 +1849,26 @@ intel_sdvo_mode_valid(struct drm_connector *connector,
> >  	struct intel_sdvo_connector *intel_sdvo_connector =
> >  		to_intel_sdvo_connector(connector);
> >  	int max_dotclk = to_i915(connector->dev)->max_dotclk_freq;
> > +	bool has_hdmi_sink = intel_has_hdmi_sink(intel_sdvo, connector->state);
> > +	int clock = mode->clock;
> >  
> >  	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> >  		return MODE_NO_DBLESCAN;
> >  
> > -	if (intel_sdvo->pixel_clock_min > mode->clock)
> > -		return MODE_CLOCK_LOW;
> >  
> > -	if (intel_sdvo->pixel_clock_max < mode->clock)
> > +	if (clock > max_dotclk)
> >  		return MODE_CLOCK_HIGH;
> >  
> > -	if (mode->clock > max_dotclk)
> > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK) {
> > +		if (!has_hdmi_sink)
> > +			return MODE_CLOCK_LOW;
> > +		clock *= 2;
> > +	}
> > +
> > +	if (intel_sdvo->pixel_clock_min > clock)
> > +		return MODE_CLOCK_LOW;
> > +
> > +	if (intel_sdvo->pixel_clock_max < clock)
> >  		return MODE_CLOCK_HIGH;
> >  
> >  	if (IS_LVDS(intel_sdvo_connector)) {
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx