| Message ID | E1YcfXV-0002uL-9O@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Russell, On 03/30/2015 03:40 PM, Russell King wrote: > Clean up hdmi_set_clk_regenerator() by allowing it to take the audio > sample rate and ratio directly, rather than hiding it inside the > function. Raise the unsupported pixel clock/sample rate message from > debug to error level as this results in audio not working correctly. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/gpu/drm/bridge/dw_hdmi.c | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c > index cca1c3d165e2..49df6c8c4ea8 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi.c > +++ b/drivers/gpu/drm/bridge/dw_hdmi.c > @@ -335,39 +335,37 @@ static unsigned int hdmi_compute_cts(unsigned int freq, unsigned long pixel_clk, > } > > static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi, > - unsigned long pixel_clk) > + unsigned long pixel_clk, unsigned int sample_rate, unsigned int ratio) > { > - unsigned int clk_n, clk_cts; > + unsigned int n, cts; > > - clk_n = hdmi_compute_n(hdmi->sample_rate, pixel_clk, > - hdmi->ratio); > - clk_cts = hdmi_compute_cts(hdmi->sample_rate, pixel_clk, > - hdmi->ratio); > - > - if (!clk_cts) { > - dev_dbg(hdmi->dev, "%s: pixel clock not supported: %lu\n", > - __func__, pixel_clk); > - return; > + n = hdmi_compute_n(sample_rate, pixel_clk, ratio); > + cts = hdmi_compute_cts(sample_rate, pixel_clk, ratio); > + if (!cts) { > + dev_err(hdmi->dev, > + "%s: pixel clock/sample rate not supported: %luMHz / %ukHz\n", > + __func__, pixel_clk, sample_rate); > } > > - dev_dbg(hdmi->dev, "%s: samplerate=%d ratio=%d pixelclk=%lu N=%d cts=%d\n", > - __func__, hdmi->sample_rate, hdmi->ratio, > - pixel_clk, clk_n, clk_cts); > + dev_dbg(hdmi->dev, "%s: samplerate=%ukHz ratio=%d pixelclk=%luMHz N=%d cts=%d\n", > + __func__, sample_rate, ratio, pixel_clk, n, cts); > > - hdmi_set_cts_n(hdmi, clk_cts, clk_n); > + hdmi_set_cts_n(hdmi, cts, n); > } > > static void hdmi_init_clk_regenerator(struct dw_hdmi *hdmi) > { > mutex_lock(&hdmi->audio_mutex); > - hdmi_set_clk_regenerator(hdmi, 74250000); > + hdmi_set_clk_regenerator(hdmi, 74250000, hdmi->sample_rate, > + hdmi->ratio); > mutex_unlock(&hdmi->audio_mutex); > } > > static void hdmi_clk_regenerator_update_pixel_clock(struct dw_hdmi *hdmi) > { > mutex_lock(&hdmi->audio_mutex); > - hdmi_set_clk_regenerator(hdmi, hdmi->hdmi_data.video_mode.mpixelclock); > + hdmi_set_clk_regenerator(hdmi, hdmi->hdmi_data.video_mode.mpixelclock, > + hdmi->sample_rate, hdmi->ratio); I'm okay with this change, and also I am preparing that collect N/CTS setting to an array, like this : struct n_cts { unsigned int cts; unsigned int n; }; struct tmds_n_cts { unsigned long tmds; /* 1 entry each for 32KHz, 44.1KHz, and 48KHz */ struct n_cts n_cts[3]; }; static const struct tmds_n_cts n_cts_table[] = { { 25175000, {{ 28125, 4576}, { 31250, 7007}, { 25175, 6144} } }, } But I am confused by the "hdmi->ratio", this variable was modify to 100 in bind funciton, then nowhere would change it again. In this case "hdmi->ratio" seems an unused variable, can we remove it ? Best regards. Yakir Yang > mutex_unlock(&hdmi->audio_mutex); > } >
On Tue, Mar 31, 2015 at 02:55:53AM -0400, Yang Kuankuan wrote: > Hi Russell, > > I'm okay with this change, and also I am preparing that collect N/CTS > setting to an array, like this : > > struct n_cts { > unsigned int cts; > unsigned int n; > }; > > struct tmds_n_cts { > unsigned long tmds; > /* 1 entry each for 32KHz, 44.1KHz, and 48KHz */ > struct n_cts n_cts[3]; > }; > > static const struct tmds_n_cts n_cts_table[] = { > { 25175000, {{ 28125, 4576}, { 31250, 7007}, { 25175, 6144} } }, > } I think this is something which should be a common helper rather than being specific to the driver. I believe these are the standard values for coherent audio/video clocks which can be found in the HDMI specifications. Such a helper should document that it is only for use when the audio and video clocks are coherent. (The HDMI spec gives alternative N values for use with auto-CTS value generation for non-coherent clocks.) I'd also prefer the lookup to use the same units as the drm_display_mode video clock specification, which is kHz, so we can eventually kill the needless *1000 in dw_hdmi. One of the issues with using a common helper is that the common helper would include the 1.001 clocks, which are allegedly unsupported by the FSL iMX6 implementation, and, if proven that they don't work, we would need some way to disable audio for those devices. > But I am confused by the "hdmi->ratio", this variable was modify to > 100 in bind funciton, then nowhere would change it again. In this > case "hdmi->ratio" seems an unused variable, can we remove it ? I guess the FSL sources should be checked to find out what the intended use for that was, and we then need to decide whether to keep it (and implement it) or remove it.
Hi Rusell, ? 2015/3/31 18:35, Russell King - ARM Linux ??: > On Tue, Mar 31, 2015 at 02:55:53AM -0400, Yang Kuankuan wrote: >> Hi Russell, >> >> I'm okay with this change, and also I am preparing that collect N/CTS >> setting to an array, like this : >> >> struct n_cts { >> unsigned int cts; >> unsigned int n; >> }; >> >> struct tmds_n_cts { >> unsigned long tmds; >> /* 1 entry each for 32KHz, 44.1KHz, and 48KHz */ >> struct n_cts n_cts[3]; >> }; >> >> static const struct tmds_n_cts n_cts_table[] = { >> { 25175000, {{ 28125, 4576}, { 31250, 7007}, { 25175, 6144} } }, >> } > I think this is something which should be a common helper rather than > being specific to the driver. I believe these are the standard values > for coherent audio/video clocks which can be found in the HDMI > specifications. Such a helper should document that it is only for > use when the audio and video clocks are coherent. Yep, it will be logical to add the n/cts calcu to a common helper. And actually the HDMI specifications have give the Revommended N and Expected CTS values for several standard TMDS clock rates(25.2/1.001 ...). > > (The HDMI spec gives alternative N values for use with auto-CTS value > generation for non-coherent clocks.) > > I'd also prefer the lookup to use the same units as the drm_display_mode > video clock specification, which is kHz, so we can eventually kill the > needless *1000 in dw_hdmi. > > One of the issues with using a common helper is that the common helper > would include the 1.001 clocks, which are allegedly unsupported by the > FSL iMX6 implementation, and, if proven that they don't work, we would > need some way to disable audio for those devices. Okay, but rockchip platform can handle the 1.001 clocks, so maybe we can call some valid function from dw_hdmi-imx & dw_hdmi-rockchip to mark the effective clock that platform support ? >> But I am confused by the "hdmi->ratio", this variable was modify to >> 100 in bind funciton, then nowhere would change it again. In this >> case "hdmi->ratio" seems an unused variable, can we remove it ? > I guess the FSL sources should be checked to find out what the intended > use for that was, and we then need to decide whether to keep it (and > implement it) or remove it. Need FSL ack... Best regards. Yakir Yang
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c index cca1c3d165e2..49df6c8c4ea8 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.c +++ b/drivers/gpu/drm/bridge/dw_hdmi.c @@ -335,39 +335,37 @@ static unsigned int hdmi_compute_cts(unsigned int freq, unsigned long pixel_clk, } static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi, - unsigned long pixel_clk) + unsigned long pixel_clk, unsigned int sample_rate, unsigned int ratio) { - unsigned int clk_n, clk_cts; + unsigned int n, cts; - clk_n = hdmi_compute_n(hdmi->sample_rate, pixel_clk, - hdmi->ratio); - clk_cts = hdmi_compute_cts(hdmi->sample_rate, pixel_clk, - hdmi->ratio); - - if (!clk_cts) { - dev_dbg(hdmi->dev, "%s: pixel clock not supported: %lu\n", - __func__, pixel_clk); - return; + n = hdmi_compute_n(sample_rate, pixel_clk, ratio); + cts = hdmi_compute_cts(sample_rate, pixel_clk, ratio); + if (!cts) { + dev_err(hdmi->dev, + "%s: pixel clock/sample rate not supported: %luMHz / %ukHz\n", + __func__, pixel_clk, sample_rate); } - dev_dbg(hdmi->dev, "%s: samplerate=%d ratio=%d pixelclk=%lu N=%d cts=%d\n", - __func__, hdmi->sample_rate, hdmi->ratio, - pixel_clk, clk_n, clk_cts); + dev_dbg(hdmi->dev, "%s: samplerate=%ukHz ratio=%d pixelclk=%luMHz N=%d cts=%d\n", + __func__, sample_rate, ratio, pixel_clk, n, cts); - hdmi_set_cts_n(hdmi, clk_cts, clk_n); + hdmi_set_cts_n(hdmi, cts, n); } static void hdmi_init_clk_regenerator(struct dw_hdmi *hdmi) { mutex_lock(&hdmi->audio_mutex); - hdmi_set_clk_regenerator(hdmi, 74250000); + hdmi_set_clk_regenerator(hdmi, 74250000, hdmi->sample_rate, + hdmi->ratio); mutex_unlock(&hdmi->audio_mutex); } static void hdmi_clk_regenerator_update_pixel_clock(struct dw_hdmi *hdmi) { mutex_lock(&hdmi->audio_mutex); - hdmi_set_clk_regenerator(hdmi, hdmi->hdmi_data.video_mode.mpixelclock); + hdmi_set_clk_regenerator(hdmi, hdmi->hdmi_data.video_mode.mpixelclock, + hdmi->sample_rate, hdmi->ratio); mutex_unlock(&hdmi->audio_mutex); }
Clean up hdmi_set_clk_regenerator() by allowing it to take the audio sample rate and ratio directly, rather than hiding it inside the function. Raise the unsupported pixel clock/sample rate message from debug to error level as this results in audio not working correctly. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/gpu/drm/bridge/dw_hdmi.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)