| Message ID | 20241201-drm-bridge-hdmi-connector-v5-8-b5316e82f61a@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: add DRM HDMI Codec framework | expand |
Hi, Sorry, I've been drowning under work and couldn't review that series before. I'll review the driver API for now, and we can focus on the exact implementation later on. On Sun, Dec 01, 2024 at 02:44:12AM +0200, Dmitry Baryshkov wrote: > Drop driver-specific implementation and use the generic HDMI Codec > framework in order to implement the HDMI audio support. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 68 ++++++++++-------------------------------- > drivers/gpu/drm/vc4/vc4_hdmi.h | 2 -- > 2 files changed, 15 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 7295834e75fb1ab0cd241ed274e675567e66870b..d0a9aff7ad43016647493263c00d593296a1e3ad 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -595,6 +595,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev, > if (vc4_hdmi->variant->supports_hdr) > max_bpc = 12; > > + connector->hdmi_codec.max_i2s_channels = 8; > + connector->hdmi_codec.i2s = 1; > + I guess it's a similar discussion than we had with HDMI2.0+ earlier today, but I don't really like initializing by structs. Struct fields are easy to miss, and can be easily uninitialized by mistake. I think I'd prefer to have them as argument to the init function. And if they are optional, we can explicitly mark them as unused. Like, it looks like the get_dai_id implementation relies on it being set to < 0 for it to be ignored, but it's not here, so I'd assume it's used with an ID of 0, even though the driver didn't support get_dai_id so far? Maxime
On Mon, Dec 02, 2024 at 02:20:04PM +0100, Maxime Ripard wrote: > Hi, > > Sorry, I've been drowning under work and couldn't review that series before. No worries, at this point I'm more concerned about my IGT series rather than this one. > > I'll review the driver API for now, and we can focus on the exact > implementation later on. > > On Sun, Dec 01, 2024 at 02:44:12AM +0200, Dmitry Baryshkov wrote: > > Drop driver-specific implementation and use the generic HDMI Codec > > framework in order to implement the HDMI audio support. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 68 ++++++++++-------------------------------- > > drivers/gpu/drm/vc4/vc4_hdmi.h | 2 -- > > 2 files changed, 15 insertions(+), 55 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index 7295834e75fb1ab0cd241ed274e675567e66870b..d0a9aff7ad43016647493263c00d593296a1e3ad 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -595,6 +595,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev, > > if (vc4_hdmi->variant->supports_hdr) > > max_bpc = 12; > > > > + connector->hdmi_codec.max_i2s_channels = 8; > > + connector->hdmi_codec.i2s = 1; > > + > > I guess it's a similar discussion than we had with HDMI2.0+ earlier > today, but I don't really like initializing by structs. Struct fields > are easy to miss, and can be easily uninitialized by mistake. > > I think I'd prefer to have them as argument to the init function. And if > they are optional, we can explicitly mark them as unused. Do you mean drm_connector_hdmi_init()? I think it's overloaded already, but I defintely can think about: drmm_connector_hdmi_init(..., max_bpc, HDMI_CODEC_I2S_PLAYBACK(8) | HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_DAI_ID(4)); or ... | HDMI_CODEC_NO_DAI_ID) The default (0) being equivalent to: HDMI_CODEC_NO_I2S | HDMI_CODEC_NO_SPDIF | HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_NO_DAI_ID WDYT? > > Like, it looks like the get_dai_id implementation relies on it being set > to < 0 for it to be ignored, but it's not here, so I'd assume it's used > with an ID of 0, even though the driver didn't support get_dai_id so > far?
On Tue, Dec 03, 2024 at 02:19:41PM +0200, Dmitry Baryshkov wrote: > On Mon, Dec 02, 2024 at 02:20:04PM +0100, Maxime Ripard wrote: > > Hi, > > > > Sorry, I've been drowning under work and couldn't review that series before. > > No worries, at this point I'm more concerned about my IGT series rather > than this one. > > > > > I'll review the driver API for now, and we can focus on the exact > > implementation later on. > > > > On Sun, Dec 01, 2024 at 02:44:12AM +0200, Dmitry Baryshkov wrote: > > > Drop driver-specific implementation and use the generic HDMI Codec > > > framework in order to implement the HDMI audio support. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 68 ++++++++++-------------------------------- > > > drivers/gpu/drm/vc4/vc4_hdmi.h | 2 -- > > > 2 files changed, 15 insertions(+), 55 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index 7295834e75fb1ab0cd241ed274e675567e66870b..d0a9aff7ad43016647493263c00d593296a1e3ad 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -595,6 +595,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev, > > > if (vc4_hdmi->variant->supports_hdr) > > > max_bpc = 12; > > > > > > + connector->hdmi_codec.max_i2s_channels = 8; > > > + connector->hdmi_codec.i2s = 1; > > > + > > > > I guess it's a similar discussion than we had with HDMI2.0+ earlier > > today, but I don't really like initializing by structs. Struct fields > > are easy to miss, and can be easily uninitialized by mistake. > > > > I think I'd prefer to have them as argument to the init function. And if > > they are optional, we can explicitly mark them as unused. > > Do you mean drm_connector_hdmi_init()? I think it's overloaded already, > but I defintely can think about: > > drmm_connector_hdmi_init(..., max_bpc, HDMI_CODEC_I2S_PLAYBACK(8) | > HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_DAI_ID(4)); > > or > > ... | HDMI_CODEC_NO_DAI_ID) > > The default (0) being equivalent to: > > HDMI_CODEC_NO_I2S | HDMI_CODEC_NO_SPDIF | HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_NO_DAI_ID > > WDYT? I know it's kind of contradictory, but it definitely looks overcrowded. A bit after we merged the HDMI infrastructure, Thomas commented that it might have been better to have a secondary init function instead of an alloc/init function. https://lore.kernel.org/all/5934b4b2-3a99-4b6b-b3e3-e57eb82b9b16@suse.de/ It's still sitting in my inbox and haven't had the time to work on that, but maybe that's how we should deal with this? Switch to using drm_connector_init, then drm_connector_hdmi_init would only take care of the video stuff, and we could have an additional drm_connector_hdmi_audio_init? That way, we could have both explicit stuff, and yet not overcrowd the arguments list too much? Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 7295834e75fb1ab0cd241ed274e675567e66870b..d0a9aff7ad43016647493263c00d593296a1e3ad 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -595,6 +595,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev, if (vc4_hdmi->variant->supports_hdr) max_bpc = 12; + connector->hdmi_codec.max_i2s_channels = 8; + connector->hdmi_codec.i2s = 1; + ret = drmm_connector_hdmi_init(dev, connector, "Broadcom", "Videocore", &vc4_hdmi_connector_funcs, @@ -1706,9 +1709,12 @@ vc4_hdmi_connector_clock_valid(const struct drm_connector *connector, return MODE_OK; } +static const struct drm_connector_hdmi_codec_funcs vc4_hdmi_codec_funcs; + static const struct drm_connector_hdmi_funcs vc4_hdmi_hdmi_connector_funcs = { .tmds_char_rate_valid = vc4_hdmi_connector_clock_valid, .write_infoframe = vc4_hdmi_write_infoframe, + .codec_funcs = &vc4_hdmi_codec_funcs, }; #define WIFI_2_4GHz_CH1_MIN_FREQ 2400000000ULL @@ -1922,9 +1928,9 @@ static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi) return true; } -static int vc4_hdmi_audio_startup(struct device *dev, void *data) +static int vc4_hdmi_audio_startup(struct drm_connector *connector) { - struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; int ret = 0; @@ -1986,9 +1992,9 @@ static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi) spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); } -static void vc4_hdmi_audio_shutdown(struct device *dev, void *data) +static void vc4_hdmi_audio_shutdown(struct drm_connector *connector) { - struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; int idx; @@ -2058,13 +2064,12 @@ static int sample_rate_to_mai_fmt(int samplerate) } /* HDMI audio codec callbacks */ -static int vc4_hdmi_audio_prepare(struct device *dev, void *data, +static int vc4_hdmi_audio_prepare(struct drm_connector *connector, struct hdmi_codec_daifmt *daifmt, struct hdmi_codec_params *params) { - struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); struct drm_device *drm = vc4_hdmi->connector.dev; - struct drm_connector *connector = &vc4_hdmi->connector; struct vc4_dev *vc4 = to_vc4_dev(drm); unsigned int sample_rate = params->sample_rate; unsigned int channels = params->channels; @@ -2076,7 +2081,7 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, int ret = 0; int idx; - dev_dbg(dev, "%s: %u Hz, %d bit, %d channels\n", __func__, + dev_dbg(&vc4_hdmi->pdev->dev, "%s: %u Hz, %d bit, %d channels\n", __func__, sample_rate, params->sample_width, channels); mutex_lock(&vc4_hdmi->mutex); @@ -2215,40 +2220,12 @@ static const struct snd_dmaengine_pcm_config pcm_conf = { .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, }; -static int vc4_hdmi_audio_get_eld(struct device *dev, void *data, - uint8_t *buf, size_t len) -{ - struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); - struct drm_connector *connector = &vc4_hdmi->connector; - - mutex_lock(&connector->eld_mutex); - memcpy(buf, connector->eld, min(sizeof(connector->eld), len)); - mutex_unlock(&connector->eld_mutex); - - return 0; -} - -static const struct hdmi_codec_ops vc4_hdmi_codec_ops = { - .get_eld = vc4_hdmi_audio_get_eld, +static const struct drm_connector_hdmi_codec_funcs vc4_hdmi_codec_funcs = { .prepare = vc4_hdmi_audio_prepare, .audio_shutdown = vc4_hdmi_audio_shutdown, .audio_startup = vc4_hdmi_audio_startup, }; -static struct hdmi_codec_pdata vc4_hdmi_codec_pdata = { - .ops = &vc4_hdmi_codec_ops, - .max_i2s_channels = 8, - .i2s = 1, -}; - -static void vc4_hdmi_audio_codec_release(void *ptr) -{ - struct vc4_hdmi *vc4_hdmi = ptr; - - platform_device_unregister(vc4_hdmi->audio.codec_pdev); - vc4_hdmi->audio.codec_pdev = NULL; -} - static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) { const struct vc4_hdmi_register *mai_data = @@ -2256,7 +2233,6 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) struct snd_soc_dai_link *dai_link = &vc4_hdmi->audio.link; struct snd_soc_card *card = &vc4_hdmi->audio.card; struct device *dev = &vc4_hdmi->pdev->dev; - struct platform_device *codec_pdev; const __be32 *addr; int index, len; int ret; @@ -2349,20 +2325,6 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) return ret; } - codec_pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME, - PLATFORM_DEVID_AUTO, - &vc4_hdmi_codec_pdata, - sizeof(vc4_hdmi_codec_pdata)); - if (IS_ERR(codec_pdev)) { - dev_err(dev, "Couldn't register the HDMI codec: %ld\n", PTR_ERR(codec_pdev)); - return PTR_ERR(codec_pdev); - } - vc4_hdmi->audio.codec_pdev = codec_pdev; - - ret = devm_add_action_or_reset(dev, vc4_hdmi_audio_codec_release, vc4_hdmi); - if (ret) - return ret; - dai_link->cpus = &vc4_hdmi->audio.cpu; dai_link->codecs = &vc4_hdmi->audio.codec; dai_link->platforms = &vc4_hdmi->audio.platform; @@ -2375,7 +2337,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) dai_link->stream_name = "MAI PCM"; dai_link->codecs->dai_name = "i2s-hifi"; dai_link->cpus->dai_name = dev_name(dev); - dai_link->codecs->name = dev_name(&codec_pdev->dev); + dai_link->codecs->name = dev_name(&vc4_hdmi->connector.hdmi_codec.codec_pdev->dev); dai_link->platforms->name = dev_name(dev); card->dai_link = dai_link; diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index b2424a21da230db99db207efa293417faccd254d..e3d989ca302b72533c374dfa3fd0d5bd7fe64a82 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -104,8 +104,6 @@ struct vc4_hdmi_audio { struct snd_soc_dai_link_component codec; struct snd_soc_dai_link_component platform; struct snd_dmaengine_dai_dma_data dma_data; - struct hdmi_audio_infoframe infoframe; - struct platform_device *codec_pdev; bool streaming; };
Drop driver-specific implementation and use the generic HDMI Codec framework in order to implement the HDMI audio support. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 68 ++++++++++-------------------------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 2 -- 2 files changed, 15 insertions(+), 55 deletions(-)