| Message ID | 20241127-media-staging-24-11-25-rb3-hw-compat-string-v1-1-99c16f266b46@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: venus: Provide support for selecting encoder/decoder from in-driver | expand |
On 27/11/2024 01:34, Bryan O'Donoghue wrote:
> + dev_info(dev, "Node %s exists won't create new\n", node_name);
Bah I meant to remove this before sending.
---
bod
On 27.11.2024 2:34 AM, Bryan O'Donoghue wrote: > Add resource structure data and probe() logic to support static > declarations of encoder and decoder. > > Right now we rely on video encoder/decoder selection happening in the dtb > but, this goes against the remit of device tree which is supposed to > describe hardware, not select functional logic in Linux drivers. > > Provide two strings in the venus resource structure enc_nodename and > dec_nodename. > > When set the venus driver will create an OF entry in-memory consistent > with: > > dec_nodename { > compat = "video-decoder"; > }; > > and/or > > enc_nodename { > compat = "video-encoder"; > }; > > This will allow us to reuse the existing driver scheme of relying on compat > names maintaining compatibility with old dtb files. > > dec_nodename can be "video-decoder" or "video0" > enc_nodename can be "video-encoder" or "video1" > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- Bryan, I'm still not sure if keeping the logic behind this makes sense at all. Could we not just call platform_device_register_data() with a static configuration of 1 core being enc and the other dec? Konrad
On 28/11/2024 20:02, Konrad Dybcio wrote: > On 27.11.2024 2:34 AM, Bryan O'Donoghue wrote: >> Add resource structure data and probe() logic to support static >> declarations of encoder and decoder. >> >> Right now we rely on video encoder/decoder selection happening in the dtb >> but, this goes against the remit of device tree which is supposed to >> describe hardware, not select functional logic in Linux drivers. >> >> Provide two strings in the venus resource structure enc_nodename and >> dec_nodename. >> >> When set the venus driver will create an OF entry in-memory consistent >> with: >> >> dec_nodename { >> compat = "video-decoder"; >> }; >> >> and/or >> >> enc_nodename { >> compat = "video-encoder"; >> }; >> >> This will allow us to reuse the existing driver scheme of relying on compat >> names maintaining compatibility with old dtb files. >> >> dec_nodename can be "video-decoder" or "video0" >> enc_nodename can be "video-encoder" or "video1" >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- > > Bryan, > > I'm still not sure if keeping the logic behind this makes sense at all. > > Could we not just call platform_device_register_data() with a static > configuration of 1 core being enc and the other dec? That's another way to do this. One reason I wrote this series to continue to rely on the existing compat= method though is it stuck in my mind that we have some dependency ordering logic in at the moment. For example: commit 08b1cf474b7f72750adebe0f0a35f8e9a3eb75f6 Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Date: Fri Feb 5 19:11:49 2021 +0100 And the other reason is the prototype platform_device_register_* looks like more surgery and ultimately more validation / potential for bugs. Since this driver is supposed to be moving to maintenance mode, I didn't want to do a more major rewrite of the probe() and remove() paths. --- bod
On 28/11/2024 22:14, Bryan O'Donoghue wrote: > On 28/11/2024 20:02, Konrad Dybcio wrote: >> Bryan, >> >> I'm still not sure if keeping the logic behind this makes sense at all. >> >> Could we not just call platform_device_register_data() with a static >> configuration of 1 core being enc and the other dec? > > That's another way to do this. One reason I wrote this series to > continue to rely on the existing compat= method though is it stuck in my > mind that we have some dependency ordering logic in at the moment. > > For example: > > commit 08b1cf474b7f72750adebe0f0a35f8e9a3eb75f6 > Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Date: Fri Feb 5 19:11:49 2021 +0100 > > And the other reason is the prototype platform_device_register_* looks > like more surgery and ultimately more validation / potential for bugs. > > Since this driver is supposed to be moving to maintenance mode, I didn't > want to do a more major rewrite of the probe() and remove() paths. > > --- > bod > and.. I wanted to continue support sdm630/sdm660/msm8996/msm8998 without any additional effort to go finding power-domains and clocks which in those cases the existing compat= method actually does something useful. Also potentially other/new additions to venus which have xcoder specific PDs and clocks would logically want to specify those as we do for the above listed SoCs. --- bod
On 28.11.2024 11:34 PM, Bryan O'Donoghue wrote: > On 28/11/2024 22:14, Bryan O'Donoghue wrote: >> On 28/11/2024 20:02, Konrad Dybcio wrote: >>> Bryan, >>> >>> I'm still not sure if keeping the logic behind this makes sense at all. >>> >>> Could we not just call platform_device_register_data() with a static >>> configuration of 1 core being enc and the other dec? >> >> That's another way to do this. One reason I wrote this series to continue to rely on the existing compat= method though is it stuck in my mind that we have some dependency ordering logic in at the moment. >> >> For example: >> >> commit 08b1cf474b7f72750adebe0f0a35f8e9a3eb75f6 >> Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> Date: Fri Feb 5 19:11:49 2021 +0100 >> >> And the other reason is the prototype platform_device_register_* looks like more surgery and ultimately more validation / potential for bugs. >> >> Since this driver is supposed to be moving to maintenance mode, I didn't want to do a more major rewrite of the probe() and remove() paths. >> >> --- >> bod >> > > and.. I wanted to continue support sdm630/sdm660/msm8996/msm8998 without any additional effort to go finding power-domains and clocks which in those cases the existing compat= method actually does something useful. > > Also potentially other/new additions to venus which have xcoder specific PDs and clocks would logically want to specify those as we do for the above listed SoCs. We can just keep a small function to grab these and assign as if they were specified under the root node (check if video-encoder/video-decoder both exist, grab the clocks for respective cores and continue normally). My old series [1] should make it easier to tackle that, feel free to take it over. Konrad [1] https://github.com/somainline/linux/commits/topic/mars
On 29/11/2024 11:23, Konrad Dybcio wrote: > We can just keep a small function to grab these and assign as if they > were specified under the root node (check if video-encoder/video-decoder > both exist, grab the clocks for respective cores and continue normally). Can do .. IMO this solution is simpler, less code and less risk of introducing upstream bugs. I'll leave it up to Vikash and Stan what solution they want. --- bod
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index 4e26b18790537885a77d66c1917a4e7a146eaf57..28fe31b8251cc0efacf43d63cb2296cf8a9c052e 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -286,6 +286,37 @@ static irqreturn_t venus_isr_thread(int irq, void *dev_id) return ret; } +static int venus_add_video_core(struct device *dev, struct of_changeset *ocs, + const char *node_name, const char *compat) +{ + struct device_node *np, *enp; + int ret; + + if (!node_name) + return 0; + + enp = of_find_node_by_name(dev->of_node, node_name); + if (enp) { + of_node_put(enp); + dev_info(dev, "Node %s exists won't create new\n", node_name); + return 0; + } + np = of_changeset_create_node(ocs, dev->of_node, node_name); + + if (!np) { + dev_err(dev, "Unable to create new node\n"); + return -ENODEV; + } + + ret = of_changeset_add_prop_string(ocs, np, "compatible", compat); + if (ret) + dev_err(dev, "unable to add %s\n", compat); + + of_node_put(np); + + return ret; +} + static int venus_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -365,6 +396,32 @@ static int venus_probe(struct platform_device *pdev) if (ret < 0) goto err_runtime_disable; + if (core->res->dec_nodename || core->res->enc_nodename) { + struct of_changeset *ocs; + + ocs = devm_kmalloc(dev, sizeof(*ocs), GFP_KERNEL); + if (!ocs) { + ret = -ENOMEM; + return ret; + } + + of_changeset_init(ocs); + + ret = venus_add_video_core(dev, ocs, core->res->dec_nodename, "venus-decoder"); + if (ret) + goto err_runtime_disable; + + ret = venus_add_video_core(dev, ocs, core->res->enc_nodename, "venus-encoder"); + if (ret) + goto err_runtime_disable; + + ret = of_changeset_apply(ocs); + if (ret) { + dev_err(dev, "applying changeset fail ret %d\n", ret); + goto err_runtime_disable; + } + } + ret = of_platform_populate(dev->of_node, NULL, NULL, dev); if (ret) goto err_runtime_disable; diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 27784fd7082c321222b23ca4b2902a04c49e19ca..4ce98a3ef186823a57ac40e2e8e42b08fafed575 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -90,6 +90,8 @@ struct venus_resources { u32 cp_nonpixel_start; u32 cp_nonpixel_size; const char *fwname; + const char *enc_nodename; + const char *dec_nodename; }; enum venus_fmt {
Add resource structure data and probe() logic to support static declarations of encoder and decoder. Right now we rely on video encoder/decoder selection happening in the dtb but, this goes against the remit of device tree which is supposed to describe hardware, not select functional logic in Linux drivers. Provide two strings in the venus resource structure enc_nodename and dec_nodename. When set the venus driver will create an OF entry in-memory consistent with: dec_nodename { compat = "video-decoder"; }; and/or enc_nodename { compat = "video-encoder"; }; This will allow us to reuse the existing driver scheme of relying on compat names maintaining compatibility with old dtb files. dec_nodename can be "video-decoder" or "video0" enc_nodename can be "video-encoder" or "video1" Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/media/platform/qcom/venus/core.c | 57 ++++++++++++++++++++++++++++++++ drivers/media/platform/qcom/venus/core.h | 2 ++ 2 files changed, 59 insertions(+)