Re: [PATCH] drm/i915/hdmi: Compute port_clock for 27.027 pixel replicated modes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 06, 2014 at 04:17:55PM -0700, Clint Taylor wrote:
> On 09/30/2014 05:46 AM, Ville Syrjälä wrote:
> > On Fri, Sep 26, 2014 at 09:28:50AM -0700, Clint Taylor wrote:
> >> On 09/26/2014 08:58 AM, Ville Syrjälä wrote:
> >>> On Wed, Sep 24, 2014 at 03:49:39PM -0700, clinton.a.taylor@intel.com wrote:
> >>>> From: Clint Taylor <clinton.a.taylor@intel.com>
> >>>>
> >>>> port_clock was being incorrectly computed and WRPLL was incorrectly
> >>>> programmed for pixel doubled modes using a 27.027MHz pixel clock.
> >>>> port_clock was set to 27.026 resulting in an output pixel clock
> >>>> matching 27.000MHz.
> >>>
> >>> I don't understand how this 27.000MHz clock would come about. Can you
> >>> explain further?
> >>
> >> I didn't really drill down into the issue once I saw the WRPLL was being
> >> computed differently for 480i@60 and the port clock was being passed as
> >> 27026.
> >>
> >>>
> >>>> Since there is no way to correctly half the 27.027
> >>>> frequency as an integer just set port_clock to 27027 for these modes.
> >>>>
> >>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/intel_hdmi.c |    5 +++++
> >>>>    1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> index 29ec153..a0786d6 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> @@ -933,6 +933,11 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>>>
> >>>>    	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
> >>>>    		pipe_config->pixel_multiplier = 2;
> >>>> +
> >>>> +		/* Fix up port_clock since 27027 is not divisible by 2 */
> >>>> +		if (pipe_config->adjusted_mode.crtc_clock == 13513) {
> >>>> +			pipe_config->port_clock = 27027;
> >>>> +		}
> >>>
> >>> Ugh. Oh well I suppose we'll want to be as accurate as possible. Would
> >>> be nice if would could someone do this fixup already in drm core, but
> >>> that would probably require changing to smaller clock units all over
> >>> the place which doesn't sound fun at all. So I guess we get to leave
> >>> the kludge in the driver :(
> >>
> >> I don't like using the 13MHz family in the mode table for this very
> >> reason. Maybe I should revisit the pixel clock setting and computation
> >> for pixel doubled modes in the i915 code.
> >
> > That has certain appeal, but it will force userspace to adjust the
> > clock for double clocked modes when it wants to calculate the refresh
> > rate. I don't really like even exposing the DBLCLK flag to userspace
> > since it's basically an internal implementation detail. Some of the
> > modes also allow higher pixel repeat factors, and we have no way to
> > inform userspace about those. So I think that forcing userspace to
> > deal with the multiplied clock is not a good idea.
> >
> >>
> >>>
> >>> This also got me thinking about the 12bpc case as well. It's clearly
> >>> busted with double clocked modes now. Care to fix that up as well?
> >>
> >> What's the best way to enter into 12bpc mode? I tried to force the mode
> >> in hdmi_compute_config(), but I wasn't able to get output.
> >
> > Looks like our 12bpc suppor is pretty broken. I did manage to make it
> > work on my IVB here but that was after I wrote several patches to fix
> > things. I don't have time to clean those up right now, but if anyone
> > else wants to work on that I can share them.
> 
> Send me the patches and I can look at the 12bpc support. My main concern 
> with 12bpc is we don't automatically switch to 12 bit when the HDMI 
> monitor supports it and nobody would take the time to manually switch to 
> 12bpc using an x11 property every time they boot. I've never had an 
> issue with sending Deep Color to a TV that supports it, but I'm also not 
> ready to try and make it automatic in the driver.

We do it automagically but we make the decision based on the primary
plane pixel format which is a bit silly considering there are sprites
and whatnot to consider. Maybe we should just always go for deep color
whenever possible? Anyway that's a separate topic.

I did manage to clean up the patches a bit while I had nothing else to
do. I'll give them a quick spin and if they still work I'll toss them
onto the mailing list.

> 
> -Clint
> 
> >
> >>
> >>>
> >>> PS. did you ever send that separate patch to fix up the interlace
> >>> flags in some of the double clocked modes? I don't remember seeing
> >>> such a patch at least.
> >>
> >> I had that on my TODO list and when I went to look at the modes (576i
> >> @200Hz) they were already correct. Magic happens!!
> >>
> >> -Clint
> >>>
> >>>>    	}
> >>>>
> >>>>    	if (intel_hdmi->color_range)
> >>>> --
> >>>> 1.7.9.5
> >>>>
> >>>> _______________________________________________
> >>>> Intel-gfx mailing list
> >>>> Intel-gfx@lists.freedesktop.org
> >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> >

-- 
Ville Syrjälä
Intel OTC