Re: [PATCH v3 03/11] drm/i915: HDMI pixel clock check

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 03/11] drm/i915: HDMI pixel clock check
Date: Fri, 14 Aug 2015 16:03:55 +0300	[thread overview]
Message-ID: <20150814130355.GY5176@intel.com> (raw)
In-Reply-To: <20150814083026.GK17734@phenom.ffwll.local>

On Fri, Aug 14, 2015 at 10:30:26AM +0200, Daniel Vetter wrote:
> On Wed, Aug 12, 2015 at 09:34:54PM +0300, Ville Syrjälä wrote:
> > On Fri, Jul 31, 2015 at 03:13:52PM +0300, Mika Kahola wrote:
> > > It is possible the we request to have a mode that has
> > > higher pixel clock than our HW can support. This patch
> > > checks if requested pixel clock is lower than the one
> > > supported by the HW. The requested mode is discarded
> > > if we cannot support the requested pixel clock.
> > > 
> > > This patch applies to HDMI.
> > > 
> > > V2:
> > > - removed computation for max dot clock
> > > 
> > > V3:
> > > - cleanup by removing unnecessary lines
> > > 
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 70bad5b..3149e5f 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1193,10 +1193,14 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> > >  	struct drm_device *dev = intel_hdmi_to_dev(hdmi);
> > >  	enum drm_mode_status status;
> > >  	int clock;
> > > +	int max_pixclk = to_i915(connector->dev)->max_dotclk;
> > >  
> > >  	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > >  		return MODE_NO_DBLESCAN;
> > >  
> > > +	if (mode->clock > max_pixclk)
> > > +		return MODE_CLOCK_HIGH;
> > > +
> > >  	clock = mode->clock;
> > 
> > I believe we should do something like this here:
> > 
> > 	clock = mode->clock;
> > 
> > 	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING)
> > 		clock *= 2;
> 
> That's already done in drm_mode_set_crtcinfo, i915 uses the STEREO_DOUBLE
> mode of that function.

Only for modesets. .mode_valid() only deals with the results of
.get_modes() etc., so the timings won't have been doubled. And even then
we only double the .crtc_ timings, which is where I previously gave up
trying to use the same mode validation code during modeset and .fill_modes().
I suppose we can make it happen if we really try, but in the meantime I
think it's at least an improvement if we stop advertising modes we for
sure can't use.

> -Daniel
> 
> > 
> > 	if (clock > max_pixclk)
> > 		return MODE_CLOCK_HIGH;
> > 
> > >  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > >  		clock *= 2;
> > 
> > The stero handling should probably be in a separate patch, but in
> > preparation for it you could already put the dotclk check between the
> > clock= assigment and the DBCLK doubling (since DBLCLK only affects the
> > port_clock and not pixel clock).
> > 
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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