| Message ID | 20161108133411.GA31754@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/11/16 13:34, Russell King - ARM Linux wrote: > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote: >> Unfortunately, my drm-tda998x-devel branch is slightly out of date with >> these patches it's the original set of 10 patches. I've not pushed >> these ones out to that branch yet, as I've three additional patches on >> top of these which aren't "ready" for pushing out. > > Here's the delta between the branch and what I just posted: Great, thanks. I committed that on top and my framebuffer console over DVI is still working. I'm dubious that it's thorough enough to have value (sadly I have neither the time nor the motivation to fight with HDMI and I2S audio), but feel free to consider that a tested-by if you wish. Robin. > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 1b262a89b7e1..3a5e5c466972 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -43,12 +43,12 @@ struct tda998x_priv { > u16 rev; > u8 current_page; > bool is_on; > - bool is_hdmi_sink; > - bool is_hdmi_config; > + bool supports_infoframes; > + bool sink_has_audio; > u8 vip_cntrl_0; > u8 vip_cntrl_1; > u8 vip_cntrl_2; > - unsigned long tdms_clock; > + unsigned long tmds_clock; > struct tda998x_audio_params audio_params; > > struct platform_device *audio_pdev; > @@ -774,7 +774,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, > * assume 100MHz requires larger divider. > */ > adiv = AUDIO_DIV_SERCLK_8; > - if (priv->tdms_clock > 100000) > + if (priv->tmds_clock > 100000) > adiv++; /* AUDIO_DIV_SERCLK_16 */ > > /* S/PDIF asks for a larger divider */ > @@ -869,8 +869,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, > } > > mutex_lock(&priv->audio_mutex); > - /* We must not program the TDA998x for audio if the sink is DVI. */ > - if (priv->is_hdmi_config) > + if (priv->supports_infoframes && priv->sink_has_audio) > ret = tda998x_configure_audio(priv, &audio); > else > ret = 0; > @@ -962,6 +961,27 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode) > return drm_helper_connector_dpms(connector, mode); > } > > +static int tda998x_connector_fill_modes(struct drm_connector *connector, > + uint32_t maxX, uint32_t maxY) > +{ > + struct tda998x_priv *priv = conn_to_tda998x_priv(connector); > + int ret; > + > + mutex_lock(&priv->audio_mutex); > + ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY); > + > + if (connector->edid_blob_ptr) { > + struct edid *edid = (void *)connector->edid_blob_ptr->data; > + > + priv->sink_has_audio = drm_detect_monitor_audio(edid); > + } else { > + priv->sink_has_audio = false; > + } > + mutex_unlock(&priv->audio_mutex); > + > + return ret; > +} > + > static enum drm_connector_status > tda998x_connector_detect(struct drm_connector *connector, bool force) > { > @@ -980,7 +1000,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector) > static const struct drm_connector_funcs tda998x_connector_funcs = { > .dpms = tda998x_connector_dpms, > .reset = drm_atomic_helper_connector_reset, > - .fill_modes = drm_helper_probe_single_connector_modes, > + .fill_modes = tda998x_connector_fill_modes, > .detect = tda998x_connector_detect, > .destroy = tda998x_connector_destroy, > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > @@ -1072,11 +1092,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector) > > drm_mode_connector_update_edid_property(connector, edid); > n = drm_add_edid_modes(connector, edid); > - priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); > - > - mutex_lock(&priv->audio_mutex); > drm_edid_to_eld(connector, edid); > - mutex_unlock(&priv->audio_mutex); > > kfree(edid); > > @@ -1350,8 +1366,22 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, > /* must be last register set: */ > reg_write(priv, REG_TBG_CNTRL_0, 0); > > - /* Only setup the info frames if the sink is HDMI */ > - if (priv->is_hdmi_sink) { > + priv->tmds_clock = adjusted_mode->clock; > + > + /* CEA-861B section 6 says that: > + * CEA version 1 (CEA-861) has no support for infoframes. > + * CEA version 2 (CEA-861A) supports version 1 AVI infoframes, > + * and optional basic audio. > + * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes, > + * and optional digital audio, with audio infoframes. > + * > + * Since we only support generation of version 2 AVI infoframes, > + * ignore CEA version 2 and below (iow, behave as if we're a > + * CEA-861 source.) > + */ > + priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3; > + > + if (priv->supports_infoframes) { > /* We need to turn HDMI HDCP stuff on to get audio through */ > reg &= ~TBG_CNTRL_1_DWIN_DIS; > reg_write(priv, REG_TBG_CNTRL_1, reg); > @@ -1360,13 +1390,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, > > tda998x_write_avi(priv, adjusted_mode); > > - priv->tdms_clock = adjusted_mode->clock; > - > - if (priv->audio_params.format != AFMT_UNUSED) > + if (priv->audio_params.format != AFMT_UNUSED && > + priv->sink_has_audio) > tda998x_configure_audio(priv, &priv->audio_params); > } > > - priv->is_hdmi_config = priv->is_hdmi_sink; > mutex_unlock(&priv->audio_mutex); > } > > >
On Tue, Nov 08, 2016 at 03:25:45PM +0000, Robin Murphy wrote: > On 08/11/16 13:34, Russell King - ARM Linux wrote: > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote: > >> Unfortunately, my drm-tda998x-devel branch is slightly out of date with > >> these patches it's the original set of 10 patches. I've not pushed > >> these ones out to that branch yet, as I've three additional patches on > >> top of these which aren't "ready" for pushing out. > > > > Here's the delta between the branch and what I just posted: > > Great, thanks. I committed that on top and my framebuffer console over > DVI is still working. I'm dubious that it's thorough enough to have > value (sadly I have neither the time nor the motivation to fight with > HDMI and I2S audio), but feel free to consider that a tested-by if you > wish. Well, "nothing obviously broken" is good enough, thanks. I don't have (or use) I2S video on my TDA998x, and that's something which is in Jyri's interest area.
On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote: > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote: > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with > > these patches it's the original set of 10 patches. I've not pushed > > these ones out to that branch yet, as I've three additional patches on > > top of these which aren't "ready" for pushing out. > > Here's the delta between the branch and what I just posted: > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c [...] I have a working setup for HDMI audio on Juno an would like to test this series but am struggling to work out which patches to apply in what order to what branch, can you be specific? (I've tried various combinations of patches series from the list, drm-tda998x-devel, and the diff you posted) Thanks
On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote: > On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote: > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote: > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with > > > these patches it's the original set of 10 patches. I've not pushed > > > these ones out to that branch yet, as I've three additional patches on > > > top of these which aren't "ready" for pushing out. > > > > Here's the delta between the branch and what I just posted: > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > [...] > > I have a working setup for HDMI audio on Juno an would like to test this > series but am struggling to work out which patches to apply in what > order to what branch, can you be specific? (I've tried various > combinations of patches series from the list, drm-tda998x-devel, and the > diff you posted) Hmm, I guess this is going to be annoyingly rather difficult then. The structure of my git tree is: v4.8 ---------------- mali patch ------------------ merge --- these patches v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^ which makes it rather difficult to send out a series that people can apply as patches without first replicating that merge. I guess the answer is... use the _patches_ for review, and I'll push out the changes into drm-tda998x-devel... should be there soon. Look for commit hash d61fa2e50f2a. (Bah, slow 'net connections.)
On Tue, 2016-11-08 at 18:24 +0000, Russell King - ARM Linux wrote: > On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote: > > On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote: > > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote: > > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with > > > > these patches it's the original set of 10 patches. I've not pushed > > > > these ones out to that branch yet, as I've three additional patches on > > > > top of these which aren't "ready" for pushing out. > > > > > > Here's the delta between the branch and what I just posted: > > > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > > [...] > > > > I have a working setup for HDMI audio on Juno an would like to test this > > series but am struggling to work out which patches to apply in what > > order to what branch, can you be specific? (I've tried various > > combinations of patches series from the list, drm-tda998x-devel, and the > > diff you posted) > > Hmm, I guess this is going to be annoyingly rather difficult then. > The structure of my git tree is: > > v4.8 ---------------- mali patch ------------------ merge --- these patches > v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^ > > which makes it rather difficult to send out a series that people can > apply as patches without first replicating that merge. I guess the > answer is... use the _patches_ for review, and I'll push out the > changes into drm-tda998x-devel... should be there soon. Look for > commit hash d61fa2e50f2a. (Bah, slow 'net connections.) Testing gets more complicated as I'm using 4.9-rc? which has a DMA fix needed for audio [1] and breaks hdmi-codec which I hope I fixed [2]. Anyway, I merged in drm-tda998x-devel and audio continued to work on my HDMI connected monitor. So I guess that's Tested-by: Jon Medhurst <tixy@linaro.org> I also reviewed the patches in this series. They look like a mostly mechanical code organisation improvement, and whilst I'm not very familiar with the driver and DRM, the other actual code changes look OK too. So, FWIW: Acked-by: Jon Medhurst <tixy@linaro.org> [1] Commit d64e9a2c7509 ("dmaengine: pl330: fix residual for non-running BUSY descriptors") [2] https://patchwork.kernel.org/patch/9401485/
On Wed, Nov 09, 2016 at 11:45:05AM +0000, Jon Medhurst (Tixy) wrote: > On Tue, 2016-11-08 at 18:24 +0000, Russell King - ARM Linux wrote: > > On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote: > > > On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote: > > > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote: > > > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with > > > > > these patches it's the original set of 10 patches. I've not pushed > > > > > these ones out to that branch yet, as I've three additional patches on > > > > > top of these which aren't "ready" for pushing out. > > > > > > > > Here's the delta between the branch and what I just posted: > > > > > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > > > [...] > > > > > > I have a working setup for HDMI audio on Juno an would like to test this > > > series but am struggling to work out which patches to apply in what > > > order to what branch, can you be specific? (I've tried various > > > combinations of patches series from the list, drm-tda998x-devel, and the > > > diff you posted) > > > > Hmm, I guess this is going to be annoyingly rather difficult then. > > The structure of my git tree is: > > > > v4.8 ---------------- mali patch ------------------ merge --- these patches > > v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^ > > > > which makes it rather difficult to send out a series that people can > > apply as patches without first replicating that merge. I guess the > > answer is... use the _patches_ for review, and I'll push out the > > changes into drm-tda998x-devel... should be there soon. Look for > > commit hash d61fa2e50f2a. (Bah, slow 'net connections.) > > Testing gets more complicated as I'm using 4.9-rc? which has a DMA fix > needed for audio [1] and breaks hdmi-codec which I hope I fixed [2]. Well, ultimately it's going to be merged during the 4.10 merge window, so it's going to be combined with lots of other changes along the way. I'm testing it against 4.8 on the Dove Cubox, and build testing it against 4.9-rc, so it's good someone is hardware testing with 4.9-rc merged. > Anyway, I merged in drm-tda998x-devel and audio continued to work on my > HDMI connected monitor. So I guess that's > > Tested-by: Jon Medhurst <tixy@linaro.org> Thanks. > I also reviewed the patches in this series. They look like a mostly > mechanical code organisation improvement, and whilst I'm not very > familiar with the driver and DRM, the other actual code changes look OK > too. So, FWIW: Yes, much of it is a code reorganisation, its mainly preparation for converting the driver to be DRM bridge based, but in reviewing the initial DRM bridge conversion patch, I spotted some weaknesses in the existing code which the first few patches of this series address. I don't think they're serious enough to warrant submission into -rc though. The next three patches I'll post in the coming days (once I've finished evaluating one of them) should reduce power consumption of the device by shutting down some blocks we never use, and some preparation to support CEC (I've had CEC working with Hans Verkil's patches for some time now, I'm mostly waiting for the CEC core to move out of staging, at which point it can end up in a saner place other than drivers/gpu/drm/i2c.) > Acked-by: Jon Medhurst <tixy@linaro.org> Thanks, and thanks for testing.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 1b262a89b7e1..3a5e5c466972 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -43,12 +43,12 @@ struct tda998x_priv { u16 rev; u8 current_page; bool is_on; - bool is_hdmi_sink; - bool is_hdmi_config; + bool supports_infoframes; + bool sink_has_audio; u8 vip_cntrl_0; u8 vip_cntrl_1; u8 vip_cntrl_2; - unsigned long tdms_clock; + unsigned long tmds_clock; struct tda998x_audio_params audio_params; struct platform_device *audio_pdev; @@ -774,7 +774,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, * assume 100MHz requires larger divider. */ adiv = AUDIO_DIV_SERCLK_8; - if (priv->tdms_clock > 100000) + if (priv->tmds_clock > 100000) adiv++; /* AUDIO_DIV_SERCLK_16 */ /* S/PDIF asks for a larger divider */ @@ -869,8 +869,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, } mutex_lock(&priv->audio_mutex); - /* We must not program the TDA998x for audio if the sink is DVI. */ - if (priv->is_hdmi_config) + if (priv->supports_infoframes && priv->sink_has_audio) ret = tda998x_configure_audio(priv, &audio); else ret = 0; @@ -962,6 +961,27 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode) return drm_helper_connector_dpms(connector, mode); } +static int tda998x_connector_fill_modes(struct drm_connector *connector, + uint32_t maxX, uint32_t maxY) +{ + struct tda998x_priv *priv = conn_to_tda998x_priv(connector); + int ret; + + mutex_lock(&priv->audio_mutex); + ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY); + + if (connector->edid_blob_ptr) { + struct edid *edid = (void *)connector->edid_blob_ptr->data; + + priv->sink_has_audio = drm_detect_monitor_audio(edid); + } else { + priv->sink_has_audio = false; + } + mutex_unlock(&priv->audio_mutex); + + return ret; +} + static enum drm_connector_status tda998x_connector_detect(struct drm_connector *connector, bool force) { @@ -980,7 +1000,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector) static const struct drm_connector_funcs tda998x_connector_funcs = { .dpms = tda998x_connector_dpms, .reset = drm_atomic_helper_connector_reset, - .fill_modes = drm_helper_probe_single_connector_modes, + .fill_modes = tda998x_connector_fill_modes, .detect = tda998x_connector_detect, .destroy = tda998x_connector_destroy, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, @@ -1072,11 +1092,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector) drm_mode_connector_update_edid_property(connector, edid); n = drm_add_edid_modes(connector, edid); - priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); - - mutex_lock(&priv->audio_mutex); drm_edid_to_eld(connector, edid); - mutex_unlock(&priv->audio_mutex); kfree(edid); @@ -1350,8 +1366,22 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, /* must be last register set: */ reg_write(priv, REG_TBG_CNTRL_0, 0); - /* Only setup the info frames if the sink is HDMI */ - if (priv->is_hdmi_sink) { + priv->tmds_clock = adjusted_mode->clock; + + /* CEA-861B section 6 says that: + * CEA version 1 (CEA-861) has no support for infoframes. + * CEA version 2 (CEA-861A) supports version 1 AVI infoframes, + * and optional basic audio. + * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes, + * and optional digital audio, with audio infoframes. + * + * Since we only support generation of version 2 AVI infoframes, + * ignore CEA version 2 and below (iow, behave as if we're a + * CEA-861 source.) + */ + priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3; + + if (priv->supports_infoframes) { /* We need to turn HDMI HDCP stuff on to get audio through */ reg &= ~TBG_CNTRL_1_DWIN_DIS; reg_write(priv, REG_TBG_CNTRL_1, reg); @@ -1360,13 +1390,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, tda998x_write_avi(priv, adjusted_mode); - priv->tdms_clock = adjusted_mode->clock; - - if (priv->audio_params.format != AFMT_UNUSED) + if (priv->audio_params.format != AFMT_UNUSED && + priv->sink_has_audio) tda998x_configure_audio(priv, &priv->audio_params); } - priv->is_hdmi_config = priv->is_hdmi_sink; mutex_unlock(&priv->audio_mutex); }