Skip to content

SKL+: need better split between HDA and non-HDA support #254

Closed
Closed
SKL+: need better split between HDA and non-HDA support#254
Assignees
keyonjie
Labels
enhancementNew feature or request

Description

@plbossart
plbossart
opened on Nov 8, 2018

The current code includes too many parts and variables from the HDAC library when SND_SOC_SOF_HDA is not defined.

See below compile-tested code only to check what is really needed

diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
index 759f1e61a030..50b0a55ce655 100644
--- a/sound/soc/sof/intel/hda-bus.c
+++ b/sound/soc/sof/intel/hda-bus.c
@@ -80,29 +80,30 @@ static const struct hdac_io_ops io_ops = {
 int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 		     const struct hdac_ext_bus_ops *ext_ops)
 {
-	static int idx;
+	static int idx; // Is this function called multiple times?
 
 	memset(bus, 0, sizeof(*bus));
 	bus->dev = dev;
 
 	bus->io_ops = &io_ops;
 	INIT_LIST_HEAD(&bus->stream_list);
-	INIT_LIST_HEAD(&bus->codec_list);
 
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
-	bus->ops = &bus_ops;
-	INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
-#endif
-	spin_lock_init(&bus->reg_lock);
-	mutex_init(&bus->cmd_mutex);
+
 	bus->irq = -1;
 
 	bus->ext_ops = ext_ops;
-	INIT_LIST_HEAD(&bus->hlink_list);
 	bus->idx = idx++;
-
 	mutex_init(&bus->lock);
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
+	INIT_LIST_HEAD(&bus->codec_list);
+	spin_lock_init(&bus->reg_lock);
+	mutex_init(&bus->cmd_mutex);
+	INIT_LIST_HEAD(&bus->hlink_list);
+	bus->ops = &bus_ops;
+	INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
 	bus->cmd_dma_state = true;
+#endif
 
 	return 0;
 }
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 42a986ce35a0..c8c0d405ef79 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -610,6 +610,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
 		return -ENOMEM;
 	}
 
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) // FIXME: where is this freed when HDA is enabled?
 	/* mem alloc for the CORB/RIRB ringbuffers */
 	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
 				  PAGE_SIZE, &bus->rb);
@@ -617,7 +618,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
 		dev_err(sdev->dev, "error: RB alloc failed\n");
 		return -ENOMEM;
 	}
-
+#endif
 	/* create capture streams */
 	for (i = 0; i < num_capture; i++) {

@keyonjie can you review and comment?

Activity

  1. keyonjie commented on Nov 8, 2018

    @keyonjie
    keyonjie
    on Nov 8, 2018

    The current code includes too many parts and variables from the HDAC library when SND_SOC_SOF_HDA is not defined.

    Good revisit. We did this in a tight schedule, it's time to refine it now.

    See below compile-tested code only to check what is really needed

    diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
index 759f1e61a030..50b0a55ce655 100644
--- a/sound/soc/sof/intel/hda-bus.c
+++ b/sound/soc/sof/intel/hda-bus.c
@@ -80,29 +80,30 @@ static const struct hdac_io_ops io_ops = {
 int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 		     const struct hdac_ext_bus_ops *ext_ops)
 {
-	static int idx;
+	static int idx; // Is this function called multiple times?

    I think this is for bus index, theoretically, it can be called multiple times then we have bus 0, bus 1, ...
    But I never come across it yet. @mengdonglin please correct me if wrong.

    memset(bus, 0, sizeof(*bus));
    bus->dev = dev;

    bus->io_ops = &io_ops;
    INIT_LIST_HEAD(&bus->stream_list);

    • INIT_LIST_HEAD(&bus->codec_list);

    -#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)

    • bus->ops = &bus_ops;
    • INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
      -#endif
    • spin_lock_init(&bus->reg_lock);
    • mutex_init(&bus->cmd_mutex);

    • bus->irq = -1;

      bus->ext_ops = ext_ops;

    • INIT_LIST_HEAD(&bus->hlink_list);
      bus->idx = idx++;
    • mutex_init(&bus->lock);

    We can even move this into HDA defined block also.

    +#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)

    • INIT_LIST_HEAD(&bus->codec_list);
    • spin_lock_init(&bus->reg_lock);
    • mutex_init(&bus->cmd_mutex);
    • INIT_LIST_HEAD(&bus->hlink_list);
    • bus->ops = &bus_ops;
    • INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
      bus->cmd_dma_state = true;
      +#endif

    Just went though all these members, agree we can handle them only when HDA defined.
    We can even move 'mutex_init(&bus->lock);' into this block also.

    return 0;
    }
    diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
    index 42a986ce35a0..c8c0d405ef79 100644
    --- a/sound/soc/sof/intel/hda-stream.c
    +++ b/sound/soc/sof/intel/hda-stream.c
    @@ -610,6 +610,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
    return -ENOMEM;
    }

    +#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) // FIXME: where is this freed when HDA is enabled?

    good catch, let's do freeing it in hda_dsp_stream_free().

    /* mem alloc for the CORB/RIRB ringbuffers */
    ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
    PAGE_SIZE, &bus->rb);
    @@ -617,7 +618,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
    dev_err(sdev->dev, "error: RB alloc failed\n");
    return -ENOMEM;
    }

    +#endif
    /* create capture streams */
    for (i = 0; i < num_capture; i++) {

    
@keyonjie can you review and comment?

    @plbossart Thanks for reviewing this part. let me do a patch to fix it.

  2. keyonjie commented on Nov 8, 2018

    @keyonjie
    keyonjie
    on Nov 8, 2018

    @plbossart PR #257 is for this, please help review.

  3. mengdonglin
    added
    enhancementNew feature or request
    on Nov 22, 2018

  4. mengdonglin commented on Nov 22, 2018

    @mengdonglin
    mengdonglin
    on Nov 22, 2018

    @plbossart @keyonjie It seems this feature can be closed since PR #257 has been merged.

  5. plbossart commented on Nov 25, 2018

    @plbossart
    plbossart
    on Nov 25, 2018
    MemberAuthor

    No, I would still like to see how we remove the static variable for the initialization of bus->idx

  6. plbossart commented on Dec 6, 2018

    @plbossart
    plbossart
    on Dec 6, 2018
    MemberAuthor

    @keyonjie can you look into this bus->idx initialization, this is the only part that needs to be cleaned.

  7. wenqingfu
    assigned
    keyonjie
    on Apr 1, 2019

  8. wenqingfu commented on Apr 1, 2019

    @wenqingfu
    wenqingfu
    on Apr 1, 2019

    @keyonjie do you agree with #300 ? Can you please follow up to finish up this one?

  9. keyonjie commented on Apr 2, 2019

    @keyonjie
    keyonjie
    on Apr 2, 2019

    just checked and found the bus->idx initialization is already supported(static and increased for each x_init() calling), the decent decreasing(freeing) of this is needed, let me do a patch for it.

  10. keyonjie commented on Apr 2, 2019

    @keyonjie
    keyonjie
    on Apr 2, 2019

    @plbossart how do you think about something like this:

    diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
index 62cc9921bb55..d924d4e3a28e 100644
--- a/sound/soc/sof/intel/hda-bus.c
+++ b/sound/soc/sof/intel/hda-bus.c
@@ -74,14 +74,34 @@ static const struct hdac_io_ops io_ops = {
        .dma_free_pages = sof_hda_dma_free_pages,
 };
 
+static u64 bus_idx_bits = 0;
+
+static int get_free_index(void)
+{
+       int i = 0;
+
+       while ((bus_idx_bits >> i) & 0x1) {
+               if (i++ == 63)
+                       return -EINVAL;
+       }
+
+       bus_idx_bits |= 1 << i;
+       return i;
+}
+
+static void put_index(int i)
+{
+       if (i < 0 || i > 63)
+               return;
+       bus_idx_bits &= ~((u64)1 << i);
+}
+
 /*
  * This can be used for both with/without hda link support.
  */
 void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
                      const struct hdac_ext_bus_ops *ext_ops)
 {
-       static int idx;
-
        memset(bus, 0, sizeof(*bus));
        bus->dev = dev;
 
@@ -90,7 +110,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 
        bus->irq = -1;
        bus->ext_ops = ext_ops;
-       bus->idx = idx++;
+       bus->idx = get_free_index();
 
        spin_lock_init(&bus->reg_lock);
 
@@ -106,3 +126,12 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 #endif
 
 }
+
+/*
+ * This can be used for both with/without hda link support.
+ */
+void sof_hda_bus_deinit(struct hdac_bus *bus)
+{
+       if (bus)
+               put_index(bus->idx);
+}
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index a57374be06e6..66405ad0a6e9 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -666,6 +666,8 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
 #endif
        hda_codec_i915_exit(sdev);
 
+       sof_hda_bus_deinit();
+
        return 0;
 }

  11. plbossart commented on Jun 10, 2019

    @plbossart
    plbossart
    on Jun 10, 2019
    MemberAuthor

    closing, this was fixed by PR#862

  12. plbossart
    closed this as completedon Jun 10, 2019
  13. plbossart
    added a commit that references this issue on Jan 13, 2020

    usb: cdns3: should not use the same dev_id for shared interrupt handler

    af58e1f
  14. wenliangwu
    added 3 commits that reference this issue on Oct 30, 2023

    selftest/bpf: Add various selftests for program limits

    d1a783d

    selftests/bpf: Test bpf_mprog query API via libbpf and raw syscall

    f9b0879

    selftests/bpf: Test query on empty mprog and pass revision into attach

    685446b
  15. plbossart
    added a commit that references this issue on Jun 12, 2024

    net/mlx5: Use mlx5_ipsec_rx_status_destroy to correctly delete status…

    16d66a4
  16. oder-chiou
    added 4 commits that reference this issue on May 5, 2026

    selftests/bpf: Add test for detachment on empty mprog entry

    21ce6ab

    selftests/bpf: Add various more tcx test cases

    ccd9a8b

    selftests/bpf: Add uprobe_multi usdt bench test

    85209e8

    selftests/bpf: Add additional mprog query test coverage

    2451630
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions