| Message ID | 1411598979-4290-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > 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 :( 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? 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. > } > > 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
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. > > 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. > > 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 >
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? > >> 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 :( > > 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? > > 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. Actually I just checked again and it was the 576i@100Hz modes and they are still incorrect. Let me send out the patch. > >> } >> >> 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 >
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. > > > > > 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 > >
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. -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 >>> >
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 > >>> > >
On Tue, Oct 07, 2014 at 11:29:30AM +0300, Ville Syrjälä wrote: > 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. Yes and yes. The one thing I'm not sure about (and why I haven't) submitted that patch is that I'm not sure about what do to with the dither bit on the different platforms.
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; + } } if (intel_hdmi->color_range)