| Message ID | 20240531-bridge-hdmi-connector-v4-1-5110f7943622@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: make use of the HDMI connector infrastructure | expand |
Hi, Sorry for not answering your mail on the previous version sooner. On Fri, May 31, 2024 at 11:07:24PM GMT, Dmitry Baryshkov wrote: > Allow passing NULL as audio infoframe as a way to disable Audio > Infoframe generation. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/display/drm_hdmi_state_helper.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > index ce96837eea65..5356723d21f5 100644 > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > @@ -681,7 +681,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_update_infoframes); > /** > * drm_atomic_helper_connector_hdmi_update_audio_infoframe - Update the Audio Infoframe > * @connector: A pointer to the HDMI connector > - * @frame: A pointer to the audio infoframe to write > + * @frame: A pointer to the audio infoframe to write or NULL to disable sending the frame I'm still two-minded about this. I think I would like a separate helper better, to also make things consistent with the HDMI helpers. Most importantly, it looks like you're not using it at all in your series? > * This function is meant for HDMI connector drivers to update their > * audio infoframe. It will typically be used in one of the ALSA hooks > @@ -704,10 +704,16 @@ drm_atomic_helper_connector_hdmi_update_audio_infoframe(struct drm_connector *co > > mutex_lock(&connector->hdmi.infoframes.lock); > > - memcpy(&infoframe->data, frame, sizeof(infoframe->data)); > - infoframe->set = true; > + if (frame) { > + memcpy(&infoframe->data, frame, sizeof(infoframe->data)); > + infoframe->set = true; > + > + ret = write_infoframe(connector, infoframe); > + } else { > + infoframe->set = false; > > - ret = write_infoframe(connector, infoframe); > + ret = clear_infoframe(connector, infoframe); > + } We should probably clear infoframe->data here too Maxime
On Mon, Jun 03, 2024 at 11:09:40AM +0200, Maxime Ripard wrote: > Hi, > > Sorry for not answering your mail on the previous version sooner. > > On Fri, May 31, 2024 at 11:07:24PM GMT, Dmitry Baryshkov wrote: > > Allow passing NULL as audio infoframe as a way to disable Audio > > Infoframe generation. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/display/drm_hdmi_state_helper.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > > index ce96837eea65..5356723d21f5 100644 > > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > > @@ -681,7 +681,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_update_infoframes); > > /** > > * drm_atomic_helper_connector_hdmi_update_audio_infoframe - Update the Audio Infoframe > > * @connector: A pointer to the HDMI connector > > - * @frame: A pointer to the audio infoframe to write > > + * @frame: A pointer to the audio infoframe to write or NULL to disable sending the frame > > I'm still two-minded about this. I think I would like a separate helper > better, to also make things consistent with the HDMI helpers. > > Most importantly, it looks like you're not using it at all in your series? It should have been a part of msm_hdmi_audio_disable(), but it seems with all the refactorings I forgot to use it. I'll check again the behaviour and either drop this patch or add a separate helper and fix other comments below. > > > * This function is meant for HDMI connector drivers to update their > > * audio infoframe. It will typically be used in one of the ALSA hooks > > @@ -704,10 +704,16 @@ drm_atomic_helper_connector_hdmi_update_audio_infoframe(struct drm_connector *co > > > > mutex_lock(&connector->hdmi.infoframes.lock); > > > > - memcpy(&infoframe->data, frame, sizeof(infoframe->data)); > > - infoframe->set = true; > > + if (frame) { > > + memcpy(&infoframe->data, frame, sizeof(infoframe->data)); > > + infoframe->set = true; > > + > > + ret = write_infoframe(connector, infoframe); > > + } else { > > + infoframe->set = false; > > > > - ret = write_infoframe(connector, infoframe); > > + ret = clear_infoframe(connector, infoframe); > > + } > > We should probably clear infoframe->data here too
diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c index ce96837eea65..5356723d21f5 100644 --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c @@ -681,7 +681,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_update_infoframes); /** * drm_atomic_helper_connector_hdmi_update_audio_infoframe - Update the Audio Infoframe * @connector: A pointer to the HDMI connector - * @frame: A pointer to the audio infoframe to write + * @frame: A pointer to the audio infoframe to write or NULL to disable sending the frame * * This function is meant for HDMI connector drivers to update their * audio infoframe. It will typically be used in one of the ALSA hooks @@ -704,10 +704,16 @@ drm_atomic_helper_connector_hdmi_update_audio_infoframe(struct drm_connector *co mutex_lock(&connector->hdmi.infoframes.lock); - memcpy(&infoframe->data, frame, sizeof(infoframe->data)); - infoframe->set = true; + if (frame) { + memcpy(&infoframe->data, frame, sizeof(infoframe->data)); + infoframe->set = true; + + ret = write_infoframe(connector, infoframe); + } else { + infoframe->set = false; - ret = write_infoframe(connector, infoframe); + ret = clear_infoframe(connector, infoframe); + } mutex_unlock(&connector->hdmi.infoframes.lock);
Allow passing NULL as audio infoframe as a way to disable Audio Infoframe generation. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/display/drm_hdmi_state_helper.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)