This is an update to
https://patchwork.kernel.org/project/linux-mediatek/list/?series=283075
To address some comments and fixes.
This series has been verified to work on 5.13.
Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
actually loading it from mmsys into two changs to make review easier.
- Update devicetree bindings to reflect no longer needing the
mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.
Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6
Changes in v4:
- rebase and test on 5.13
- don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name
Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
mediatek,vpu property inside the mdp_rdma0 device node.
Eizan Miyamoto (9):
mtk-mdp: propagate errors from clock_on
mtk-mdp: add driver to probe mdp components
mtk-mdp: use pm_runtime in MDP component driver
media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on
mtk-mdp: make mdp driver to be loadable by platform_device_register*()
soc: mediatek: mmsys: instantiate mdp virtual device from mmsys
media: mtk-mdp: use mdp-rdma0 alias to point to MDP master
dts: mtk-mdp: remove mediatek,vpu property from primary MDP device
dt-bindings: mediatek: remove vpu requirement from mtk-mdp
.../bindings/media/mediatek-mdp.txt | 3 -
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 -
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 268 +++++++++++++++--
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 34 +--
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 282 ++++++++++++------
drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 3 +
drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 4 +-
drivers/soc/mediatek/mtk-mmsys.c | 20 +-
8 files changed, 470 insertions(+), 145 deletions(-)
--
2.32.0.554.ge1b32706d8-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Up until this change, many errors were logged but ignored when powering
on clocks inside mtk_mdp_core. This change tries to do a better job at
propagating errors back to the power management framework.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 +-
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++-----
3 files changed, 39 insertions(+), 15 deletions(-)
Hi Eizan,
Thank you for your patch.
Missatge de Eizan Miyamoto [off-list ref] del dia dl., 2 d’ag.
2021 a les 14:13:
Up until this change, many errors were logged but ignored when powering
on clocks inside mtk_mdp_core. This change tries to do a better job at
propagating errors back to the power management framework.
Signed-off-by: Eizan Miyamoto <redacted>
Up until this change, many errors were logged but ignored when powering
on clocks inside mtk_mdp_core. This change tries to do a better job at
propagating errors back to the power management framework.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 +-
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++-----
3 files changed, 39 insertions(+), 15 deletions(-)
Hi Dafna, thank you very much for spending time to review the patch,
your time spent is very much appreciated.
On Thu, Aug 5, 2021 at 4:06 PM Dafna Hirschfeld
[off-list ref] wrote:
quoted
+err_clk_prepare_enable:
+ for (--i; i >= 0; i--) {
+ if (IS_ERR(comp->clk[i]))
+ continue;
+ clk_disable_unprepare(comp->clk[i]);
}
+
+ return status;
There is an API function clk_bulk_prepare_enable to prepare and enable an array of clks
so you can just use it.
As per Enric's suggestion earlier in this email thread, are you OK
with me making this change in a follow-up patch, particularly since
the logic as it is was preserved from previous functionality?
You can get rid of the new var 'status' and just return ret in case of error
This seems like a nit (please let me know if you disagree), and it's
also cleaned up in a follow-on patch in the series ("don't
pm_run_time_get/put for master comp in clock_on"). Is making the
change you are suggesting here something that should require uploading
a new series version?
Eizan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Dafna, thank you very much for spending time to review the patch,
your time spent is very much appreciated.
On Thu, Aug 5, 2021 at 4:06 PM Dafna Hirschfeld
[off-list ref] wrote:
quoted
quoted
+err_clk_prepare_enable:
+ for (--i; i >= 0; i--) {
+ if (IS_ERR(comp->clk[i]))
+ continue;
+ clk_disable_unprepare(comp->clk[i]);
}
+
+ return status;
There is an API function clk_bulk_prepare_enable to prepare and enable an array of clks
so you can just use it.
As per Enric's suggestion earlier in this email thread, are you OK
with me making this change in a follow-up patch, particularly since
the logic as it is was preserved from previous functionality?
sure,
I just give suggestions.
A follow-up patch would be nice.
You can get rid of the new var 'status' and just return ret in case of error
This seems like a nit (please let me know if you disagree), and it's
also cleaned up in a follow-on patch in the series ("don't
pm_run_time_get/put for master comp in clock_on"). Is making the
change you are suggesting here something that should require uploading
a new series version?
Hi, no, I am fine with it. No need for new version.
Thanks,
Dafna
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
Up until this change, many errors were logged but ignored when
powering
on clocks inside mtk_mdp_core. This change tries to do a better job
at
propagating errors back to the power management framework.
Signed-off-by: Eizan Miyamoto <redacted>
---
Broadly, this patch (1) adds a driver for various MTK MDP components to
go alongside the main MTK MDP driver, and (2) hooks them all together
using the component framework.
(1) Up until now, the MTK MDP driver controls 8 devices in the device
tree on its own. When running tests for the hardware video decoder, we
found that the iommus and LARBs were not being properly configured. To
configure them, a driver for each be added to mtk_mdp_comp so that
mtk_iommu_add_device() can (eventually) be called from dma_configure()
inside really_probe().
(2) The integration into the component framework allows us to defer the
registration with the v4l2 subsystem until all the MDP-related devices
have been probed, so that the relevant device node does not become
available until initialization of all the components is complete.
Some notes about how the component framework has been integrated:
- The driver for the rdma0 component serves double duty as the "master"
(aggregate) driver as well as a component driver. This is a non-ideal
compromise until a better solution is developed. This device is
differentiated from the rest by checking for a "mediatek,vpu" property
in the device node.
- The list of mdp components remains hard-coded as mtk_mdp_comp_dt_ids[]
in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
mtk_mdp_comp.c. This unfortunate duplication of information is
addressed in a following patch in this series.
- The component driver calls component_add() for each device that is
probed.
- In mtk_mdp_probe (the "master" device), we scan the device tree for
any matching nodes against mtk_mdp_comp_dt_ids, and add component
matches for them. The match criteria is a matching device node
pointer.
- When the set of components devices that have been probed corresponds
with the list that is generated by the "master", the callback to
mtk_mdp_master_bind() is made, which then calls the component bind
functions.
- Inside mtk_mdp_master_bind(), once all the component bind functions
have been called, we can then register our device to the v4l2
subsystem.
- The call to pm_runtime_enable() in the master device is called after
all the components have been registered by their bind() functions
called by mtk_mtp_master_bind(). As a result, the list of components
will not change while power management callbacks mtk_mdp_suspend()/
resume() are accessing the list of components.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 143 ++++++++++++--
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 25 +--
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 174 +++++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 1 +
4 files changed, 252 insertions(+), 91 deletions(-)
@@ -20,9 +57,7 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)if(comp->larb_dev){err=mtk_smi_larb_get(comp->larb_dev);if(err)-dev_err(dev,-"failed to get larb, err %d. type:%d\n",-err,comp->type);+dev_err(dev,"failed to get larb, err %d.\n",err);}for(i=0;i<ARRAY_SIZE(comp->clk);i++){
@@ -80,19 +139,17 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,if(PTR_ERR(comp->clk[i])!=-EPROBE_DEFER)dev_err(dev,"Failed to get clock\n");ret=PTR_ERR(comp->clk[i]);-gotoput_dev;+gotoerr;}/* Only RDMA needs two clocks */-if(comp->type!=MTK_MDP_RDMA)+if(comp_type!=MTK_MDP_RDMA)break;}/* Only DMA capable components need the LARB property */comp->larb_dev=NULL;-if(comp->type!=MTK_MDP_RDMA&&-comp->type!=MTK_MDP_WDMA&&-comp->type!=MTK_MDP_WROT)+if(comp_type!=MTK_MDP_RDMA&&comp_type!=MTK_MDP_WDMA&&comp_type!=MTK_MDP_WROT)return0;larb_node=of_parse_phandle(node,"mediatek,larb",0);
@@ -100,7 +157,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,dev_err(dev,"Missing mediadek,larb phandle in %pOF node\n",node);ret=-EINVAL;-gotoput_dev;+gotoerr;}larb_pdev=of_find_device_by_node(larb_node);
@@ -108,7 +165,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,dev_warn(dev,"Waiting for larb device %pOF\n",larb_node);of_node_put(larb_node);ret=-EPROBE_DEFER;-gotoput_dev;+gotoerr;}of_node_put(larb_node);
@@ -116,13 +173,59 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,return0;-put_dev:-of_node_put(comp->dev_node);-+err:returnret;}-voidmtk_mdp_comp_deinit(structdevice*dev,structmtk_mdp_comp*comp)+staticintmtk_mdp_comp_probe(structplatform_device*pdev){-of_node_put(comp->dev_node);+structdevice*dev=&pdev->dev;+structdevice_node*vpu_node;+intstatus;+structmtk_mdp_comp*comp;++vpu_node=of_parse_phandle(dev->of_node,"mediatek,vpu",0);+if(vpu_node){+of_node_put(vpu_node);+/*+*Thedevicetreenodewithamediatek,vpupropertyisdeemed+*theMDP"master"device,wedon'twanttoaddacomponent+*foritinthisfunctionbecausetheinitializationforthe+*masterisdoneelsewhere.+*/+dev_info(dev,"vpu node found, not probing\n");+return-ENODEV;+}++comp=devm_kzalloc(dev,sizeof(*comp),GFP_KERNEL);+if(!comp)+return-ENOMEM;++status=mtk_mdp_comp_init(comp,dev);+if(status){+dev_err(dev,"Failed to initialize component: %d\n",status);+returnstatus;+}++dev_set_drvdata(dev,comp);++returncomponent_add(dev,&mtk_mdp_component_ops);}++staticintmtk_mdp_comp_remove(structplatform_device*pdev)+{+structdevice*dev=&pdev->dev;++component_del(dev,&mtk_mdp_component_ops);+return0;+}++structplatform_drivermtk_mdp_component_driver={+.probe=mtk_mdp_comp_probe,+.remove=mtk_mdp_comp_remove,+.driver={+.name="mediatek-mdp-comp",+.owner=THIS_MODULE,+.of_match_table=mtk_mdp_comp_driver_dt_match,+},+};
Hi Eizan,
Thank you for your patch.
Missatge de Eizan Miyamoto [off-list ref] del dia dl., 2 d’ag.
2021 a les 14:13:
Broadly, this patch (1) adds a driver for various MTK MDP components to
go alongside the main MTK MDP driver, and (2) hooks them all together
using the component framework.
(1) Up until now, the MTK MDP driver controls 8 devices in the device
tree on its own. When running tests for the hardware video decoder, we
found that the iommus and LARBs were not being properly configured. To
configure them, a driver for each be added to mtk_mdp_comp so that
mtk_iommu_add_device() can (eventually) be called from dma_configure()
inside really_probe().
(2) The integration into the component framework allows us to defer the
registration with the v4l2 subsystem until all the MDP-related devices
have been probed, so that the relevant device node does not become
available until initialization of all the components is complete.
Some notes about how the component framework has been integrated:
- The driver for the rdma0 component serves double duty as the "master"
(aggregate) driver as well as a component driver. This is a non-ideal
compromise until a better solution is developed. This device is
differentiated from the rest by checking for a "mediatek,vpu" property
in the device node.
- The list of mdp components remains hard-coded as mtk_mdp_comp_dt_ids[]
in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
mtk_mdp_comp.c. This unfortunate duplication of information is
addressed in a following patch in this series.
- The component driver calls component_add() for each device that is
probed.
- In mtk_mdp_probe (the "master" device), we scan the device tree for
any matching nodes against mtk_mdp_comp_dt_ids, and add component
matches for them. The match criteria is a matching device node
pointer.
- When the set of components devices that have been probed corresponds
with the list that is generated by the "master", the callback to
mtk_mdp_master_bind() is made, which then calls the component bind
functions.
- Inside mtk_mdp_master_bind(), once all the component bind functions
have been called, we can then register our device to the v4l2
subsystem.
- The call to pm_runtime_enable() in the master device is called after
all the components have been registered by their bind() functions
called by mtk_mtp_master_bind(). As a result, the list of components
will not change while power management callbacks mtk_mdp_suspend()/
resume() are accessing the list of components.
Signed-off-by: Eizan Miyamoto <redacted>
@@ -20,9 +57,7 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)if(comp->larb_dev){err=mtk_smi_larb_get(comp->larb_dev);if(err)-dev_err(dev,-"failed to get larb, err %d. type:%d\n",-err,comp->type);+dev_err(dev,"failed to get larb, err %d.\n",err);}for(i=0;i<ARRAY_SIZE(comp->clk);i++){
@@ -80,19 +139,17 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,if(PTR_ERR(comp->clk[i])!=-EPROBE_DEFER)dev_err(dev,"Failed to get clock\n");ret=PTR_ERR(comp->clk[i]);-gotoput_dev;+gotoerr;}/* Only RDMA needs two clocks */-if(comp->type!=MTK_MDP_RDMA)+if(comp_type!=MTK_MDP_RDMA)break;}/* Only DMA capable components need the LARB property */comp->larb_dev=NULL;-if(comp->type!=MTK_MDP_RDMA&&-comp->type!=MTK_MDP_WDMA&&-comp->type!=MTK_MDP_WROT)+if(comp_type!=MTK_MDP_RDMA&&comp_type!=MTK_MDP_WDMA&&comp_type!=MTK_MDP_WROT)return0;larb_node=of_parse_phandle(node,"mediatek,larb",0);
@@ -100,7 +157,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,dev_err(dev,"Missing mediadek,larb phandle in %pOF node\n",node);ret=-EINVAL;-gotoput_dev;+gotoerr;}larb_pdev=of_find_device_by_node(larb_node);
@@ -108,7 +165,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,dev_warn(dev,"Waiting for larb device %pOF\n",larb_node);of_node_put(larb_node);ret=-EPROBE_DEFER;-gotoput_dev;+gotoerr;}of_node_put(larb_node);
@@ -116,13 +173,59 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,return0;-put_dev:-of_node_put(comp->dev_node);-+err:returnret;}-voidmtk_mdp_comp_deinit(structdevice*dev,structmtk_mdp_comp*comp)+staticintmtk_mdp_comp_probe(structplatform_device*pdev){-of_node_put(comp->dev_node);+structdevice*dev=&pdev->dev;+structdevice_node*vpu_node;+intstatus;+structmtk_mdp_comp*comp;++vpu_node=of_parse_phandle(dev->of_node,"mediatek,vpu",0);+if(vpu_node){+of_node_put(vpu_node);+/*+*Thedevicetreenodewithamediatek,vpupropertyisdeemed+*theMDP"master"device,wedon'twanttoaddacomponent+*foritinthisfunctionbecausetheinitializationforthe+*masterisdoneelsewhere.+*/+dev_info(dev,"vpu node found, not probing\n");+return-ENODEV;+}++comp=devm_kzalloc(dev,sizeof(*comp),GFP_KERNEL);+if(!comp)+return-ENOMEM;++status=mtk_mdp_comp_init(comp,dev);+if(status){+dev_err(dev,"Failed to initialize component: %d\n",status);+returnstatus;+}++dev_set_drvdata(dev,comp);++returncomponent_add(dev,&mtk_mdp_component_ops);}++staticintmtk_mdp_comp_remove(structplatform_device*pdev)+{+structdevice*dev=&pdev->dev;++component_del(dev,&mtk_mdp_component_ops);+return0;+}++structplatform_drivermtk_mdp_component_driver={+.probe=mtk_mdp_comp_probe,+.remove=mtk_mdp_comp_remove,+.driver={+.name="mediatek-mdp-comp",+.owner=THIS_MODULE,+.of_match_table=mtk_mdp_comp_driver_dt_match,+},+};
Broadly, this patch (1) adds a driver for various MTK MDP components to
go alongside the main MTK MDP driver, and (2) hooks them all together
using the component framework.
(1) Up until now, the MTK MDP driver controls 8 devices in the device
tree on its own. When running tests for the hardware video decoder, we
found that the iommus and LARBs were not being properly configured. To
configure them, a driver for each be added to mtk_mdp_comp so that
mtk_iommu_add_device() can (eventually) be called from dma_configure()
inside really_probe().
(2) The integration into the component framework allows us to defer the
registration with the v4l2 subsystem until all the MDP-related devices
have been probed, so that the relevant device node does not become
available until initialization of all the components is complete.
Some notes about how the component framework has been integrated:
- The driver for the rdma0 component serves double duty as the "master"
(aggregate) driver as well as a component driver. This is a non-ideal
compromise until a better solution is developed. This device is
differentiated from the rest by checking for a "mediatek,vpu" property
in the device node.
- The list of mdp components remains hard-coded as mtk_mdp_comp_dt_ids[]
in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
mtk_mdp_comp.c. This unfortunate duplication of information is
addressed in a following patch in this series.
- The component driver calls component_add() for each device that is
probed.
- In mtk_mdp_probe (the "master" device), we scan the device tree for
any matching nodes against mtk_mdp_comp_dt_ids, and add component
matches for them. The match criteria is a matching device node
pointer.
- When the set of components devices that have been probed corresponds
with the list that is generated by the "master", the callback to
mtk_mdp_master_bind() is made, which then calls the component bind
functions.
- Inside mtk_mdp_master_bind(), once all the component bind functions
have been called, we can then register our device to the v4l2
subsystem.
- The call to pm_runtime_enable() in the master device is called after
all the components have been registered by their bind() functions
called by mtk_mtp_master_bind(). As a result, the list of components
will not change while power management callbacks mtk_mdp_suspend()/
resume() are accessing the list of components.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 143 ++++++++++++--
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 25 +--
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 174 +++++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 1 +
4 files changed, 252 insertions(+), 91 deletions(-)
@@ -20,9 +57,7 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)if(comp->larb_dev){err=mtk_smi_larb_get(comp->larb_dev);if(err)-dev_err(dev,-"failed to get larb, err %d. type:%d\n",-err,comp->type);+dev_err(dev,"failed to get larb, err %d.\n",err);}for(i=0;i<ARRAY_SIZE(comp->clk);i++){
If it's all the same to you, are you okay for me to make this change either:
- If upstream requests changes be made to this series, I will include
this suggestion
- If the series is accepted as-is, I will make a follow-up patch
This is to reduce workload on reviewers having to re-ack changes for
(what I hope you agree is) a nit.
If you don't agree, I will happily oblige and make the change and
upload a new version of the series.
quoted
for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
comp->clk[i] = of_clk_get(node, i);
that iteration can be replaced with clk_bulk_get
As per Enric's suggestion in response to "propagate errors from
clock_on" in this series, are you okay for me to make the change in a
follow-up patch?
Thanks again for your review,
Eizan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
If it's all the same to you, are you okay for me to make this change either:
- If upstream requests changes be made to this series, I will include
this suggestion
- If the series is accepted as-is, I will make a follow-up patch
This is to reduce workload on reviewers having to re-ack changes for
(what I hope you agree is) a nit.
I think you can keep the review-by tag of the reviewers since this is just a nit.
So sending a new version won't need a re-ack from the reviewers.
You can send it also as a separate patch. I don't care too much.
If you don't agree, I will happily oblige and make the change and
upload a new version of the series.
quoted
quoted
for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
comp->clk[i] = of_clk_get(node, i);
that iteration can be replaced with clk_bulk_get
As per Enric's suggestion in response to "propagate errors from
clock_on" in this series, are you okay for me to make the change in a
follow-up patch?
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
Broadly, this patch (1) adds a driver for various MTK MDP components
to
go alongside the main MTK MDP driver, and (2) hooks them all together
using the component framework.
(1) Up until now, the MTK MDP driver controls 8 devices in the device
tree on its own. When running tests for the hardware video decoder,
we
found that the iommus and LARBs were not being properly configured.
To
configure them, a driver for each be added to mtk_mdp_comp so that
mtk_iommu_add_device() can (eventually) be called from
dma_configure()
inside really_probe().
(2) The integration into the component framework allows us to defer
the
registration with the v4l2 subsystem until all the MDP-related
devices
have been probed, so that the relevant device node does not become
available until initialization of all the components is complete.
Some notes about how the component framework has been integrated:
- The driver for the rdma0 component serves double duty as the
"master"
(aggregate) driver as well as a component driver. This is a non-
ideal
compromise until a better solution is developed. This device is
differentiated from the rest by checking for a "mediatek,vpu"
property
in the device node.
- The list of mdp components remains hard-coded as
mtk_mdp_comp_dt_ids[]
in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
mtk_mdp_comp.c. This unfortunate duplication of information is
addressed in a following patch in this series.
- The component driver calls component_add() for each device that is
probed.
- In mtk_mdp_probe (the "master" device), we scan the device tree for
any matching nodes against mtk_mdp_comp_dt_ids, and add component
matches for them. The match criteria is a matching device node
pointer.
- When the set of components devices that have been probed
corresponds
with the list that is generated by the "master", the callback to
mtk_mdp_master_bind() is made, which then calls the component bind
functions.
- Inside mtk_mdp_master_bind(), once all the component bind functions
have been called, we can then register our device to the v4l2
subsystem.
- The call to pm_runtime_enable() in the master device is called
after
all the components have been registered by their bind() functions
called by mtk_mtp_master_bind(). As a result, the list of
components
will not change while power management callbacks mtk_mdp_suspend()/
resume() are accessing the list of components.
Signed-off-by: Eizan Miyamoto <redacted>
---
Without this change, the MDP components are not fully integrated into
the runtime power management subsystem, and the MDP driver does not
work.
For each of the component device drivers to be able to call
pm_runtime_get/put_sync() a pointer to the component's device struct
had to be added to struct mtk_mdp_comp, set by mtk_mdp_comp_init().
Note that the dev argument to mtk_mdp_comp_clock_on/off() has been
removed. Those functions used to be called from the "master" mdp driver
in mtk_mdp_core.c, but the component's device pointer no longer
corresponds to the mdp master device pointer, which is not the right
device to pass to pm_runtime_put/get_sync() which we had to add to get
the driver to work properly.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 24 +++++++++++++++----
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 6 +++--
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 7 +++---
3 files changed, 27 insertions(+), 10 deletions(-)
Hi Eizan,
Thank you for your patch.
Missatge de Eizan Miyamoto [off-list ref] del dia dl., 2 d’ag.
2021 a les 14:14:
Without this change, the MDP components are not fully integrated into
the runtime power management subsystem, and the MDP driver does not
work.
For each of the component device drivers to be able to call
pm_runtime_get/put_sync() a pointer to the component's device struct
had to be added to struct mtk_mdp_comp, set by mtk_mdp_comp_init().
Note that the dev argument to mtk_mdp_comp_clock_on/off() has been
removed. Those functions used to be called from the "master" mdp driver
in mtk_mdp_core.c, but the component's device pointer no longer
corresponds to the mdp master device pointer, which is not the right
device to pass to pm_runtime_put/get_sync() which we had to add to get
the driver to work properly.
Signed-off-by: Eizan Miyamoto <redacted>
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
Without this change, the MDP components are not fully integrated into
the runtime power management subsystem, and the MDP driver does not
work.
For each of the component device drivers to be able to call
pm_runtime_get/put_sync() a pointer to the component's device struct
had to be added to struct mtk_mdp_comp, set by mtk_mdp_comp_init().
Note that the dev argument to mtk_mdp_comp_clock_on/off() has been
removed. Those functions used to be called from the "master" mdp
driver
in mtk_mdp_core.c, but the component's device pointer no longer
corresponds to the mdp master device pointer, which is not the right
device to pass to pm_runtime_put/get_sync() which we had to add to
get
the driver to work properly.
Signed-off-by: Eizan Miyamoto <redacted>
---
The original intent of commit 86698b9505bbc ("media: mtk-mdp: convert
mtk_mdp_dev.comp array to list") was to create a list to track all the
MDP components that needed to have their clocks enabled/disabled when
calling mtk_mdp_comp_clock_on/off. However, there was a bug inside
mtk_mdp_register_component where the args to a call to list_add were
swapped. The result is that only one component was added to
mtk_mdp_dev.comp_list because comp_list was instead being
repeatedly added to the single element lists headed by each
mtk_mdp_comp.
The order of the args to list_add in mtk_mdp_register_component was
fixed in https://patchwork.kernel.org/patch/11742895/ (Fix Null pointer
dereference when calling list_add).
Then, as a result of https://patchwork.kernel.org/patch/11530769/
(mtk-mdp: use pm_runtime in MDP component driver) if all the components
are added to the component list, the mdp "master" / rdma0 component
ends up having pm_runtime_get_sync() called on it twice recursively:
rpm_resume+0x694/0x8f8
__pm_runtime_resume+0x7c/0xa0 ***NOTE***
mtk_mdp_comp_clock_on+0x48/0x104 [mtk_mdp]
mtk_mdp_pm_resume+0x2c/0x44 [mtk_mdp]
pm_generic_runtime_resume+0x34/0x48
__genpd_runtime_resume+0x6c/0x80
genpd_runtime_resume+0x104/0x1ac
__rpm_callback+0x120/0x238
rpm_callback+0x34/0x8c
rpm_resume+0x7a0/0x8f8
__pm_runtime_resume+0x7c/0xa0 ***NOTE***
mtk_mdp_m2m_start_streaming+0x2c/0x3c [mtk_mdp]
(The calls to pm_runtime_get_sync are inlined and correspond to the
calls to __pm_runtime_resume). It is not correct to have
pm_runtime_get_sync called recursively and the second call will block
indefinitely.
As a result of all that, this change factors mtk_mdp_comp_clock_on/off
into mtk_mdp_comp_power_on/off and moves the calls to
pm_runtime_get/put into the power_on/off functions.
This change then special-cases the master/rdma0 MDP component and does
these things:
- the master/rdma0 component is not added to mtk_mdp_dev.comp_list
- the master/rdma0 component has its clocks (*but not power*) toggled
by mtk_mpd_comp_clock_on/off inside mtk_mdp_clock_on/off.
- the other components have their clocks *and* power toggled with
mtk_mdp_comp_power_on/off.
This change introduces the assumption that mtk_mdp_pm_resume will
always be called though a callback from pm_runtime_get_sync made on the
master / rdma0 component.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 57 ++++++++++++++---
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 5 +-
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++++++++++-----
3 files changed, 98 insertions(+), 26 deletions(-)
@@ -63,12 +63,54 @@ int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)err=pm_runtime_get_sync(comp->dev);if(err<0){-dev_err(comp->dev,-"failed to runtime get, err %d.\n",-err);+dev_err(comp->dev,"failed to runtime get, err %d.\n",err);returnerr;}+err=mtk_mdp_comp_clock_on(comp);+if(err){+dev_err(comp->dev,"failed to turn on clock. err=%d",err);+status=err;+gotoerr_mtk_mdp_comp_clock_on;+}++return0;++err_mtk_mdp_comp_clock_on:+err=pm_runtime_put_sync(comp->dev);+if(err)+dev_err(comp->dev,"failed to runtime put in cleanup. err=%d",err);++returnstatus;+}++intmtk_mdp_comp_power_off(structmtk_mdp_comp*comp)+{+intstatus,err;++mtk_mdp_comp_clock_off(comp);++err=pm_runtime_put_sync(comp->dev);+if(err<0){+dev_err(comp->dev,"failed to runtime put, err %d.\n",err);+status=err;+gotoerr_pm_runtime_put_sync;+}++return0;++err_pm_runtime_put_sync:+err=mtk_mdp_comp_clock_on(comp);+if(err)+dev_err(comp->dev,"failed to turn on clock in cleanup. err=%d",err);++returnstatus;+}++intmtk_mdp_comp_clock_on(structmtk_mdp_comp*comp)+{+inti,err,status;+for(i=0;i<ARRAY_SIZE(comp->clk);i++){if(IS_ERR(comp->clk[i]))continue;
@@ -94,7 +136,8 @@ int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)returnstatus;}-intmtk_mdp_comp_clock_off(structmtk_mdp_comp*comp)++voidmtk_mdp_comp_clock_off(structmtk_mdp_comp*comp){inti;
@@ -106,8 +149,6 @@ int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)if(comp->larb_dev)mtk_smi_larb_put(comp->larb_dev);--returnpm_runtime_put_sync(comp->dev);}staticintmtk_mdp_comp_bind(structdevice*dev,structdevice*master,void*data)
@@ -57,29 +57,64 @@ static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)structdevice*dev=&mdp->pdev->dev;interr;+/*+*Themaster/rdma0componentwillhavepm_runtime_get_synccalled+*onitthroughmtk_mdp_m2m_start_streaming,makingitunnecessaryto+*havemtk_mdp_comp_power_oncalledonit.+*/+err=mtk_mdp_comp_clock_on(&mdp->comp_self);+if(err)+returnerr;+list_for_each_entry(comp_node,&mdp->comp_list,node){-err=mtk_mdp_comp_clock_on(comp_node);+err=mtk_mdp_comp_power_on(comp_node);if(err){status=err;-gotoerr_mtk_mdp_comp_clock_on;+gotoerr_mtk_mdp_comp_power_on;}}return0;-err_mtk_mdp_comp_clock_on:-list_for_each_entry_continue_reverse(comp_node,&mdp->comp_list,node)-mtk_mdp_comp_clock_off(comp_node);-+err_mtk_mdp_comp_power_on:+list_for_each_entry_continue_reverse(comp_node,&mdp->comp_list,node){+err=mtk_mdp_comp_power_off(comp_node);+if(err)+dev_err(&mdp->pdev->dev,"failed to power off after error. err=%d",err);+}returnstatus;}-staticvoidmtk_mdp_clock_off(structmtk_mdp_dev*mdp)+staticintmtk_mdp_clock_off(structmtk_mdp_dev*mdp){structmtk_mdp_comp*comp_node;+intstatus,err;++list_for_each_entry(comp_node,&mdp->comp_list,node){+err=mtk_mdp_comp_power_off(comp_node);+if(err){+status=err;+gotoerr_mtk_mdp_comp_power_off;+}+}-list_for_each_entry(comp_node,&mdp->comp_list,node)-mtk_mdp_comp_clock_off(comp_node);+/*+*Themaster/rdma0componentwillhavepm_runtime_putcalled+*onitthroughmtk_mdp_m2m_stop_streaming,makingitunnecessaryto+*havemtk_mdp_comp_power_offcalledonit.+*/+mtk_mdp_comp_clock_off(&mdp->comp_self);++return0;++err_mtk_mdp_comp_power_off:+list_for_each_entry_continue_reverse(comp_node,&mdp->comp_list,node){+err=mtk_mdp_comp_power_on(comp_node);+if(err)+dev_err(&mdp->pdev->dev,"failed to power on after error. err=%d",err);+}++returnstatus;}staticvoidmtk_mdp_wdt_worker(structwork_struct*work)
@@ -118,8 +153,6 @@ static int mtk_mdp_master_bind(struct device *dev)intstatus;structmtk_mdp_dev*mdp=dev_get_drvdata(dev);-mtk_mdp_register_component(mdp,&mdp->comp_self);-status=component_bind_all(dev,mdp);if(status){dev_err(dev,"Failed to bind all components: %d\n",status);
@@ -140,8 +173,6 @@ static int mtk_mdp_master_bind(struct device *dev)component_unbind_all(dev,mdp);err_component_bind_all:-mtk_mdp_unregister_component(mdp,&mdp->comp_self);-returnstatus;}
Hi Eizan,
Thank you for your patch.
Missatge de Eizan Miyamoto [off-list ref] del dia dl., 2 d’ag.
2021 a les 14:13:
quoted hunk
The original intent of commit 86698b9505bbc ("media: mtk-mdp: convert
mtk_mdp_dev.comp array to list") was to create a list to track all the
MDP components that needed to have their clocks enabled/disabled when
calling mtk_mdp_comp_clock_on/off. However, there was a bug inside
mtk_mdp_register_component where the args to a call to list_add were
swapped. The result is that only one component was added to
mtk_mdp_dev.comp_list because comp_list was instead being
repeatedly added to the single element lists headed by each
mtk_mdp_comp.
The order of the args to list_add in mtk_mdp_register_component was
fixed in https://patchwork.kernel.org/patch/11742895/ (Fix Null pointer
dereference when calling list_add).
Then, as a result of https://patchwork.kernel.org/patch/11530769/
(mtk-mdp: use pm_runtime in MDP component driver) if all the components
are added to the component list, the mdp "master" / rdma0 component
ends up having pm_runtime_get_sync() called on it twice recursively:
rpm_resume+0x694/0x8f8
__pm_runtime_resume+0x7c/0xa0 ***NOTE***
mtk_mdp_comp_clock_on+0x48/0x104 [mtk_mdp]
mtk_mdp_pm_resume+0x2c/0x44 [mtk_mdp]
pm_generic_runtime_resume+0x34/0x48
__genpd_runtime_resume+0x6c/0x80
genpd_runtime_resume+0x104/0x1ac
__rpm_callback+0x120/0x238
rpm_callback+0x34/0x8c
rpm_resume+0x7a0/0x8f8
__pm_runtime_resume+0x7c/0xa0 ***NOTE***
mtk_mdp_m2m_start_streaming+0x2c/0x3c [mtk_mdp]
(The calls to pm_runtime_get_sync are inlined and correspond to the
calls to __pm_runtime_resume). It is not correct to have
pm_runtime_get_sync called recursively and the second call will block
indefinitely.
As a result of all that, this change factors mtk_mdp_comp_clock_on/off
into mtk_mdp_comp_power_on/off and moves the calls to
pm_runtime_get/put into the power_on/off functions.
This change then special-cases the master/rdma0 MDP component and does
these things:
- the master/rdma0 component is not added to mtk_mdp_dev.comp_list
- the master/rdma0 component has its clocks (*but not power*) toggled
by mtk_mpd_comp_clock_on/off inside mtk_mdp_clock_on/off.
- the other components have their clocks *and* power toggled with
mtk_mdp_comp_power_on/off.
This change introduces the assumption that mtk_mdp_pm_resume will
always be called though a callback from pm_runtime_get_sync made on the
master / rdma0 component.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 57 ++++++++++++++---
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 5 +-
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++++++++++-----
3 files changed, 98 insertions(+), 26 deletions(-)
@@ -63,12 +63,54 @@ int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)err=pm_runtime_get_sync(comp->dev);if(err<0){-dev_err(comp->dev,-"failed to runtime get, err %d.\n",-err);+dev_err(comp->dev,"failed to runtime get, err %d.\n",err);returnerr;}+err=mtk_mdp_comp_clock_on(comp);+if(err){+dev_err(comp->dev,"failed to turn on clock. err=%d",err);+status=err;+gotoerr_mtk_mdp_comp_clock_on;+}++return0;++err_mtk_mdp_comp_clock_on:+err=pm_runtime_put_sync(comp->dev);+if(err)+dev_err(comp->dev,"failed to runtime put in cleanup. err=%d",err);++returnstatus;+}++intmtk_mdp_comp_power_off(structmtk_mdp_comp*comp)+{+intstatus,err;++mtk_mdp_comp_clock_off(comp);++err=pm_runtime_put_sync(comp->dev);+if(err<0){+dev_err(comp->dev,"failed to runtime put, err %d.\n",err);+status=err;+gotoerr_pm_runtime_put_sync;+}++return0;++err_pm_runtime_put_sync:+err=mtk_mdp_comp_clock_on(comp);+if(err)+dev_err(comp->dev,"failed to turn on clock in cleanup. err=%d",err);++returnstatus;+}++intmtk_mdp_comp_clock_on(structmtk_mdp_comp*comp)+{+inti,err,status;+for(i=0;i<ARRAY_SIZE(comp->clk);i++){if(IS_ERR(comp->clk[i]))continue;
@@ -94,7 +136,8 @@ int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)returnstatus;}-intmtk_mdp_comp_clock_off(structmtk_mdp_comp*comp)++voidmtk_mdp_comp_clock_off(structmtk_mdp_comp*comp){inti;
@@ -106,8 +149,6 @@ int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)if(comp->larb_dev)mtk_smi_larb_put(comp->larb_dev);--returnpm_runtime_put_sync(comp->dev);}staticintmtk_mdp_comp_bind(structdevice*dev,structdevice*master,void*data)
@@ -57,29 +57,64 @@ static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)structdevice*dev=&mdp->pdev->dev;interr;+/*+*Themaster/rdma0componentwillhavepm_runtime_get_synccalled+*onitthroughmtk_mdp_m2m_start_streaming,makingitunnecessaryto+*havemtk_mdp_comp_power_oncalledonit.+*/+err=mtk_mdp_comp_clock_on(&mdp->comp_self);+if(err)+returnerr;+list_for_each_entry(comp_node,&mdp->comp_list,node){-err=mtk_mdp_comp_clock_on(comp_node);+err=mtk_mdp_comp_power_on(comp_node);if(err){status=err;-gotoerr_mtk_mdp_comp_clock_on;+gotoerr_mtk_mdp_comp_power_on;}}return0;-err_mtk_mdp_comp_clock_on:-list_for_each_entry_continue_reverse(comp_node,&mdp->comp_list,node)-mtk_mdp_comp_clock_off(comp_node);-+err_mtk_mdp_comp_power_on:+list_for_each_entry_continue_reverse(comp_node,&mdp->comp_list,node){+err=mtk_mdp_comp_power_off(comp_node);+if(err)+dev_err(&mdp->pdev->dev,"failed to power off after error. err=%d",err);+}returnstatus;}-staticvoidmtk_mdp_clock_off(structmtk_mdp_dev*mdp)+staticintmtk_mdp_clock_off(structmtk_mdp_dev*mdp){structmtk_mdp_comp*comp_node;+intstatus,err;++list_for_each_entry(comp_node,&mdp->comp_list,node){+err=mtk_mdp_comp_power_off(comp_node);+if(err){+status=err;+gotoerr_mtk_mdp_comp_power_off;+}+}-list_for_each_entry(comp_node,&mdp->comp_list,node)-mtk_mdp_comp_clock_off(comp_node);+/*+*Themaster/rdma0componentwillhavepm_runtime_putcalled+*onitthroughmtk_mdp_m2m_stop_streaming,makingitunnecessaryto+*havemtk_mdp_comp_power_offcalledonit.+*/+mtk_mdp_comp_clock_off(&mdp->comp_self);++return0;++err_mtk_mdp_comp_power_off:+list_for_each_entry_continue_reverse(comp_node,&mdp->comp_list,node){+err=mtk_mdp_comp_power_on(comp_node);+if(err)+dev_err(&mdp->pdev->dev,"failed to power on after error. err=%d",err);+}++returnstatus;}staticvoidmtk_mdp_wdt_worker(structwork_struct*work)
@@ -118,8 +153,6 @@ static int mtk_mdp_master_bind(struct device *dev)intstatus;structmtk_mdp_dev*mdp=dev_get_drvdata(dev);-mtk_mdp_register_component(mdp,&mdp->comp_self);-status=component_bind_all(dev,mdp);if(status){dev_err(dev,"Failed to bind all components: %d\n",status);
@@ -140,8 +173,6 @@ static int mtk_mdp_master_bind(struct device *dev)component_unbind_all(dev,mdp);err_component_bind_all:-mtk_mdp_unregister_component(mdp,&mdp->comp_self);-returnstatus;}
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
The original intent of commit 86698b9505bbc ("media: mtk-mdp: convert
mtk_mdp_dev.comp array to list") was to create a list to track all
the
MDP components that needed to have their clocks enabled/disabled when
calling mtk_mdp_comp_clock_on/off. However, there was a bug inside
mtk_mdp_register_component where the args to a call to list_add were
swapped. The result is that only one component was added to
mtk_mdp_dev.comp_list because comp_list was instead being
repeatedly added to the single element lists headed by each
mtk_mdp_comp.
The order of the args to list_add in mtk_mdp_register_component was
fixed in https://patchwork.kernel.org/patch/11742895/ (Fix Null
pointer
dereference when calling list_add).
Then, as a result of https://patchwork.kernel.org/patch/11530769/
(mtk-mdp: use pm_runtime in MDP component driver) if all the
components
are added to the component list, the mdp "master" / rdma0 component
ends up having pm_runtime_get_sync() called on it twice recursively:
rpm_resume+0x694/0x8f8
__pm_runtime_resume+0x7c/0xa0 ***NOTE***
mtk_mdp_comp_clock_on+0x48/0x104 [mtk_mdp]
mtk_mdp_pm_resume+0x2c/0x44 [mtk_mdp]
pm_generic_runtime_resume+0x34/0x48
__genpd_runtime_resume+0x6c/0x80
genpd_runtime_resume+0x104/0x1ac
__rpm_callback+0x120/0x238
rpm_callback+0x34/0x8c
rpm_resume+0x7a0/0x8f8
__pm_runtime_resume+0x7c/0xa0 ***NOTE***
mtk_mdp_m2m_start_streaming+0x2c/0x3c [mtk_mdp]
(The calls to pm_runtime_get_sync are inlined and correspond to the
calls to __pm_runtime_resume). It is not correct to have
pm_runtime_get_sync called recursively and the second call will block
indefinitely.
As a result of all that, this change factors
mtk_mdp_comp_clock_on/off
into mtk_mdp_comp_power_on/off and moves the calls to
pm_runtime_get/put into the power_on/off functions.
This change then special-cases the master/rdma0 MDP component and
does
these things:
- the master/rdma0 component is not added to mtk_mdp_dev.comp_list
- the master/rdma0 component has its clocks (*but not power*) toggled
by mtk_mpd_comp_clock_on/off inside mtk_mdp_clock_on/off.
- the other components have their clocks *and* power toggled with
mtk_mdp_comp_power_on/off.
This change introduces the assumption that mtk_mdp_pm_resume will
always be called though a callback from pm_runtime_get_sync made on
the
master / rdma0 component.
Signed-off-by: Eizan Miyamoto <redacted>
---
Rather than hanging the MDP master component driver off of the rdma0
device, make it possible too create a "virtual" device by registering
it with platform_device_register_*() to be probed by the mtk_mdp_core
driver.
Broadly, three interdependent things are done by this change:
- Make it is possible to search for MDP devices in the device tree
through the grandparent device's of_node.
- v4l-related setup is moved into from the mtk_mdp_core driver to the
mtk_mdp_comp driver.
- Presence of a mediatek,vpu property in an MDP component device node
is used to determine what device to use when dispatching DMA ops from
the relevant ioctl, and also do V4L2 initialization in this case.
Signed-off-by: Eizan Miyamoto <redacted>
---
Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
actually loading it from mmsys into two changs to make review easier.
- Update devicetree bindings to reflect no longer needing the
mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.
Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6
Changes in v4:
- rebase and test on 5.13
- don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name
Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
mediatek,vpu property inside the mdp_rdma0 device node.
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 51 ++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++-------------
drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 2 +
drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 4 +-
4 files changed, 60 insertions(+), 59 deletions(-)
@@ -159,6 +159,17 @@ static int mtk_mdp_master_bind(struct device *dev)gotoerr_component_bind_all;}+if(mdp->vpu_dev){+intret=vpu_wdt_reg_handler(mdp->vpu_dev,mtk_mdp_reset_handler,mdp,+VPU_RST_MDP);+if(ret){+dev_err(dev,"Failed to register reset handler\n");+gotoerr_wdt_reg;+}+}else{+dev_err(dev,"no vpu_dev found\n");+}+status=mtk_mdp_register_m2m_device(mdp);if(status){dev_err(dev,"Failed to register m2m device: %d\n",status);
@@ -170,6 +181,8 @@ static int mtk_mdp_master_bind(struct device *dev)return0;err_mtk_mdp_register_m2m_device:++err_wdt_reg:component_unbind_all(dev,mdp);err_component_bind_all:
@@ -228,8 +241,13 @@ static int mtk_mdp_probe(struct platform_device *pdev)of_node_put(node);parent=dev->of_node;dev_warn(dev,"device tree is out of date\n");-}else{+}elseif(dev->of_node){parent=dev->of_node->parent;+}elseif(dev->parent){+/* maybe we were created from a call to platform_device_register_data() */+parent=dev->parent->parent->of_node;+}else{+return-ENODEV;}/* Iterate over sibling MDP function blocks */
@@ -262,16 +280,6 @@ static int mtk_mdp_probe(struct platform_device *pdev)}}-/*-*Createacomponentformyselfsothatclockscanbetoggledin-*clock_on().-*/-ret=mtk_mdp_comp_init(&mdp->comp_self,dev);-if(ret){-dev_err(dev,"Failed to initialize component\n");-gotoerr_comp;-}-mdp->job_wq=create_singlethread_workqueue(MTK_MDP_MODULE_NAME);if(!mdp->job_wq){dev_err(&pdev->dev,"unable to alloc job workqueue\n");
@@ -287,29 +295,8 @@ static int mtk_mdp_probe(struct platform_device *pdev)}INIT_WORK(&mdp->wdt_work,mtk_mdp_wdt_worker);-ret=v4l2_device_register(dev,&mdp->v4l2_dev);-if(ret){-dev_err(&pdev->dev,"Failed to register v4l2 device\n");-ret=-EINVAL;-gotoerr_dev_register;-}--mdp->vpu_dev=vpu_get_plat_device(pdev);-ret=vpu_wdt_reg_handler(mdp->vpu_dev,mtk_mdp_reset_handler,mdp,-VPU_RST_MDP);-if(ret){-dev_err(&pdev->dev,"Failed to register reset handler\n");-gotoerr_wdt_reg;-}-platform_set_drvdata(pdev,mdp);-ret=vb2_dma_contig_set_max_seg_size(&pdev->dev,DMA_BIT_MASK(32));-if(ret){-dev_err(&pdev->dev,"Failed to set vb2 dma mag seg size\n");-gotoerr_set_max_seg_size;-}-ret=component_master_add_with_match(dev,&mtk_mdp_com_ops,match);if(ret){dev_err(dev,"Component master add failed\n");
@@ -321,22 +308,12 @@ static int mtk_mdp_probe(struct platform_device *pdev)return0;err_component_master_add:-vb2_dma_contig_clear_max_seg_size(&pdev->dev);--err_set_max_seg_size:--err_wdt_reg:-v4l2_device_unregister(&mdp->v4l2_dev);--err_dev_register:destroy_workqueue(mdp->wdt_wq);err_alloc_wdt_wq:destroy_workqueue(mdp->job_wq);err_alloc_job_wq:--err_comp:dev_dbg(dev,"err %d\n",ret);returnret;}
Hi Eizan,
Thank you for your patch.
Missatge de Eizan Miyamoto [off-list ref] del dia dl., 2 d’ag.
2021 a les 14:13:
quoted hunk
Rather than hanging the MDP master component driver off of the rdma0
device, make it possible too create a "virtual" device by registering
it with platform_device_register_*() to be probed by the mtk_mdp_core
driver.
Broadly, three interdependent things are done by this change:
- Make it is possible to search for MDP devices in the device tree
through the grandparent device's of_node.
- v4l-related setup is moved into from the mtk_mdp_core driver to the
mtk_mdp_comp driver.
- Presence of a mediatek,vpu property in an MDP component device node
is used to determine what device to use when dispatching DMA ops from
the relevant ioctl, and also do V4L2 initialization in this case.
Signed-off-by: Eizan Miyamoto <redacted>
---
Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
actually loading it from mmsys into two changs to make review easier.
- Update devicetree bindings to reflect no longer needing the
mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.
Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6
Changes in v4:
- rebase and test on 5.13
- don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name
Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
mediatek,vpu property inside the mdp_rdma0 device node.
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 51 ++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++-------------
drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 2 +
drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 4 +-
4 files changed, 60 insertions(+), 59 deletions(-)
This looks a bit excessive IMO, but on the other hand looks like this
is a transitional patch as all this will be removed after some rework
on the latest patch.
+ dev_err(dev, "vpu pdev failed\n");
You already did a WARN_ON, this print is not needed. But again, all
this seems to be transitional and is removed later. So it doesn't
really bothers me
Reviewed-by: Enric Balletbo i Serra <redacted>
quoted hunk
+ of_node_put(vpu_node);
+ }
+
+ ret = v4l2_device_register(dev, &mdp->v4l2_dev);
+ if (ret) {
+ dev_err(dev, "Failed to register v4l2 device\n");
+ return -EINVAL;
+ }
+
+ ret = vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
+ if (ret) {
+ dev_err(dev, "Failed to set vb2 dma mag seg size\n");
+ return -EINVAL;
+ }
+
+ /*
+ * presence of the "mediatek,vpu" property in a device node
+ * indicates that it is the primary MDP rdma device and MDP DMA
+ * ops should be handled by its DMA callbacks.
+ */
+ mdp->rdma_dev = dev;
+ }
+
pm_runtime_enable(dev);
return 0;
@@ -237,23 +274,9 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev) static int mtk_mdp_comp_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev;- struct device_node *vpu_node; int status; struct mtk_mdp_comp *comp;- vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);- if (vpu_node) {- of_node_put(vpu_node);- /*- * The device tree node with a mediatek,vpu property is deemed- * the MDP "master" device, we don't want to add a component- * for it in this function because the initialization for the- * master is done elsewhere.- */- dev_info(dev, "vpu node found, not probing\n");- return -ENODEV;- }- comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL); if (!comp) return -ENOMEM;
@@ -159,6 +159,17 @@ static int mtk_mdp_master_bind(struct device *dev)gotoerr_component_bind_all;}+if(mdp->vpu_dev){+intret=vpu_wdt_reg_handler(mdp->vpu_dev,mtk_mdp_reset_handler,mdp,+VPU_RST_MDP);+if(ret){+dev_err(dev,"Failed to register reset handler\n");+gotoerr_wdt_reg;+}+}else{+dev_err(dev,"no vpu_dev found\n");+}+status=mtk_mdp_register_m2m_device(mdp);if(status){dev_err(dev,"Failed to register m2m device: %d\n",status);
@@ -170,6 +181,8 @@ static int mtk_mdp_master_bind(struct device *dev)return0;err_mtk_mdp_register_m2m_device:++err_wdt_reg:component_unbind_all(dev,mdp);err_component_bind_all:
@@ -228,8 +241,13 @@ static int mtk_mdp_probe(struct platform_device *pdev)of_node_put(node);parent=dev->of_node;dev_warn(dev,"device tree is out of date\n");-}else{+}elseif(dev->of_node){parent=dev->of_node->parent;+}elseif(dev->parent){+/* maybe we were created from a call to platform_device_register_data() */+parent=dev->parent->parent->of_node;+}else{+return-ENODEV;}/* Iterate over sibling MDP function blocks */
@@ -262,16 +280,6 @@ static int mtk_mdp_probe(struct platform_device *pdev)}}-/*-*Createacomponentformyselfsothatclockscanbetoggledin-*clock_on().-*/-ret=mtk_mdp_comp_init(&mdp->comp_self,dev);-if(ret){-dev_err(dev,"Failed to initialize component\n");-gotoerr_comp;-}-mdp->job_wq=create_singlethread_workqueue(MTK_MDP_MODULE_NAME);if(!mdp->job_wq){dev_err(&pdev->dev,"unable to alloc job workqueue\n");
@@ -287,29 +295,8 @@ static int mtk_mdp_probe(struct platform_device *pdev)}INIT_WORK(&mdp->wdt_work,mtk_mdp_wdt_worker);-ret=v4l2_device_register(dev,&mdp->v4l2_dev);-if(ret){-dev_err(&pdev->dev,"Failed to register v4l2 device\n");-ret=-EINVAL;-gotoerr_dev_register;-}--mdp->vpu_dev=vpu_get_plat_device(pdev);-ret=vpu_wdt_reg_handler(mdp->vpu_dev,mtk_mdp_reset_handler,mdp,-VPU_RST_MDP);-if(ret){-dev_err(&pdev->dev,"Failed to register reset handler\n");-gotoerr_wdt_reg;-}-platform_set_drvdata(pdev,mdp);-ret=vb2_dma_contig_set_max_seg_size(&pdev->dev,DMA_BIT_MASK(32));-if(ret){-dev_err(&pdev->dev,"Failed to set vb2 dma mag seg size\n");-gotoerr_set_max_seg_size;-}-ret=component_master_add_with_match(dev,&mtk_mdp_com_ops,match);if(ret){dev_err(dev,"Component master add failed\n");
@@ -321,22 +308,12 @@ static int mtk_mdp_probe(struct platform_device *pdev)return0;err_component_master_add:-vb2_dma_contig_clear_max_seg_size(&pdev->dev);--err_set_max_seg_size:--err_wdt_reg:-v4l2_device_unregister(&mdp->v4l2_dev);--err_dev_register:destroy_workqueue(mdp->wdt_wq);err_alloc_wdt_wq:destroy_workqueue(mdp->job_wq);err_alloc_job_wq:--err_comp:dev_dbg(dev,"err %d\n",ret);returnret;}
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
Rather than hanging the MDP master component driver off of the rdma0
device, make it possible too create a "virtual" device by registering
it with platform_device_register_*() to be probed by the mtk_mdp_core
driver.
Broadly, three interdependent things are done by this change:
- Make it is possible to search for MDP devices in the device tree
through the grandparent device's of_node.
- v4l-related setup is moved into from the mtk_mdp_core driver to the
mtk_mdp_comp driver.
- Presence of a mediatek,vpu property in an MDP component device node
is used to determine what device to use when dispatching DMA ops
from
the relevant ioctl, and also do V4L2 initialization in this case.
Signed-off-by: Eizan Miyamoto <redacted>
---
Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
actually loading it from mmsys into two changs to make review
easier.
- Update devicetree bindings to reflect no longer needing the
mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.
Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6
Changes in v4:
- rebase and test on 5.13
- don't depend on
https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name
Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
mediatek,vpu property inside the mdp_rdma0 device node.
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
quoted hunk
Rather than hanging the MDP master component driver off of the rdma0
device, make it possible too create a "virtual" device by registering
it with platform_device_register_*() to be probed by the mtk_mdp_core
driver.
Broadly, three interdependent things are done by this change:
- Make it is possible to search for MDP devices in the device tree
through the grandparent device's of_node.
- v4l-related setup is moved into from the mtk_mdp_core driver to the
mtk_mdp_comp driver.
- Presence of a mediatek,vpu property in an MDP component device node
is used to determine what device to use when dispatching DMA ops
from
the relevant ioctl, and also do V4L2 initialization in this case.
Signed-off-by: Eizan Miyamoto <redacted>
---
Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
actually loading it from mmsys into two changs to make review
easier.
- Update devicetree bindings to reflect no longer needing the
mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.
Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6
Changes in v4:
- rebase and test on 5.13
- don't depend on
https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name
Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
mediatek,vpu property inside the mdp_rdma0 device node.
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 51 ++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++-----------
--
drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 2 +
drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 4 +-
4 files changed, 60 insertions(+), 59 deletions(-)
@@ -155,8 +155,45 @@ static int mtk_mdp_comp_bind(struct device *dev,
struct device *master, void *da
{
struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
struct mtk_mdp_dev *mdp = data;
+ struct device_node *vpu_node;
mtk_mdp_register_component(mdp, comp);
+
+ /*
+ * If this component has a "mediatek-vpu" property, it is
responsible for
+ * notifying the mdp master driver about it so it can be
further initialized
+ * later.
+ */
+ vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
+ if (vpu_node) {
+ int ret;
+
+ mdp->vpu_dev = of_find_device_by_node(vpu_node);
+ if (WARN_ON(!mdp->vpu_dev)) {
+ dev_err(dev, "vpu pdev failed\n");
+ of_node_put(vpu_node);
+ }
Maybe 'of_node_put(vpu_node)' should be called when 'mdp->vpu_dev' is
valid.
quoted hunk
+ ret = v4l2_device_register(dev, &mdp->v4l2_dev);
+ if (ret) {
+ dev_err(dev, "Failed to register v4l2
device\n");
+ return -EINVAL;
+ }
+
+ ret = vb2_dma_contig_set_max_seg_size(dev,
DMA_BIT_MASK(32));
+ if (ret) {
+ dev_err(dev, "Failed to set vb2 dma mag seg
size\n");
+ return -EINVAL;
+ }
+
+ /*
+ * presence of the "mediatek,vpu" property in a device
node
+ * indicates that it is the primary MDP rdma device and
MDP DMA
+ * ops should be handled by its DMA callbacks.
+ */
+ mdp->rdma_dev = dev;
+ }
+
pm_runtime_enable(dev);
return 0;
@@ -237,23 +274,9 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp,
struct device *dev)
static int mtk_mdp_comp_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct device_node *vpu_node;
int status;
struct mtk_mdp_comp *comp;
- vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
- if (vpu_node) {
- of_node_put(vpu_node);
- /*
- * The device tree node with a mediatek,vpu property is
deemed
- * the MDP "master" device, we don't want to add a
component
- * for it in this function because the initialization
for the
- * master is done elsewhere.
- */
- dev_info(dev, "vpu node found, not probing\n");
- return -ENODEV;
- }
-
comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
if (!comp)
return -ENOMEM;
@@ -228,8 +241,13 @@ static int mtk_mdp_probe(struct platform_device
*pdev)
of_node_put(node);
parent = dev->of_node;
dev_warn(dev, "device tree is out of date\n");
- } else {
+ } else if (dev->of_node) {
parent = dev->of_node->parent;
+ } else if (dev->parent) {
+ /* maybe we were created from a call to
platform_device_register_data() */
+ parent = dev->parent->parent->of_node;
+ } else {
+ return -ENODEV;
}
/* Iterate over sibling MDP function blocks */
@@ -262,16 +280,6 @@ static int mtk_mdp_probe(struct platform_device
*pdev)
}
}
- /*
- * Create a component for myself so that clocks can be toggled
in
- * clock_on().
- */
- ret = mtk_mdp_comp_init(&mdp->comp_self, dev);
- if (ret) {
- dev_err(dev, "Failed to initialize component\n");
- goto err_comp;
- }
-
mdp->job_wq =
create_singlethread_workqueue(MTK_MDP_MODULE_NAME);
if (!mdp->job_wq) {
dev_err(&pdev->dev, "unable to alloc job workqueue\n");
@@ -287,29 +295,8 @@ static int mtk_mdp_probe(struct platform_device
*pdev)
}
INIT_WORK(&mdp->wdt_work, mtk_mdp_wdt_worker);
- ret = v4l2_device_register(dev, &mdp->v4l2_dev);
- if (ret) {
- dev_err(&pdev->dev, "Failed to register v4l2
device\n");
- ret = -EINVAL;
- goto err_dev_register;
- }
-
- mdp->vpu_dev = vpu_get_plat_device(pdev);
- ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler,
mdp,
- VPU_RST_MDP);
- if (ret) {
- dev_err(&pdev->dev, "Failed to register reset
handler\n");
- goto err_wdt_reg;
- }
-
platform_set_drvdata(pdev, mdp);
- ret = vb2_dma_contig_set_max_seg_size(&pdev->dev,
DMA_BIT_MASK(32));
- if (ret) {
- dev_err(&pdev->dev, "Failed to set vb2 dma mag seg
size\n");
- goto err_set_max_seg_size;
- }
-
ret = component_master_add_with_match(dev, &mtk_mdp_com_ops,
match);
if (ret) {
dev_err(dev, "Component master add failed\n");
@@ -321,22 +308,12 @@ static int mtk_mdp_probe(struct platform_device
A virtual device that is probed by the mtk_mdp_core driver is
instantiated by the mtk_mmsys driver.
This better reflects the logical organization of the hardware and
driver: there are a number of hardware blocks that are used by the MDP
that have no strict hierarchy, and the software driver is responsible
for driving them properly.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/soc/mediatek/mtk-mmsys.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
Hi Eizan,
Thank you for your patch.
Missatge de Eizan Miyamoto [off-list ref] del dia dl., 2 d’ag.
2021 a les 14:14:
A virtual device that is probed by the mtk_mdp_core driver is
instantiated by the mtk_mmsys driver.
This better reflects the logical organization of the hardware and
driver: there are a number of hardware blocks that are used by the MDP
that have no strict hierarchy, and the software driver is responsible
for driving them properly.
Signed-off-by: Eizan Miyamoto <redacted>
Reviewed-by: Enric Balletbo i Serra <redacted>
quoted hunk
---
(no changes since v1)
drivers/soc/mediatek/mtk-mmsys.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
A virtual device that is probed by the mtk_mdp_core driver is
instantiated by the mtk_mmsys driver.
This better reflects the logical organization of the hardware and
driver: there are a number of hardware blocks that are used by the
MDP
that have no strict hierarchy, and the software driver is responsible
for driving them properly.
Signed-off-by: Eizan Miyamoto <redacted>
---
... Instead of depending on the presence of a mediatek,vpu property in
the device node.
That property was originally added to link to the vpu node so that the
mtk_mdp_core driver could pass the right device to
vpu_wdt_reg_handler(). However in a previous patch in this series,
the driver has been modified to search the device tree for that node
instead.
That property was also used to indicate the primary MDP device, so that
it can be passed to the V4L2 subsystem as well as register it to be
used when setting up queues in the open() callback for the filesystem
device node that is created. In this case, assuming that the primary
MDP device is the one with a specific alias seems useable because the
alternative is to add a property to the device tree which doesn't
actually represent any facet of hardware (i.e., this being the primary
MDP device is a software decision). In other words, this solution is
equally as arbitrary, but at least it doesn't add a property to a
device node where said property is unrelated to the hardware present.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
2 files changed, 64 insertions(+), 28 deletions(-)
Hi Eizan,
Thank you for your patch.
Missatge de Eizan Miyamoto [off-list ref] del dia dl., 2 d’ag.
2021 a les 14:14:
... Instead of depending on the presence of a mediatek,vpu property in
Looks like there is something missing in the commit message?
the device node.
That property was originally added to link to the vpu node so that the
mtk_mdp_core driver could pass the right device to
vpu_wdt_reg_handler(). However in a previous patch in this series,
the driver has been modified to search the device tree for that node
instead.
That property was also used to indicate the primary MDP device, so that
it can be passed to the V4L2 subsystem as well as register it to be
used when setting up queues in the open() callback for the filesystem
device node that is created. In this case, assuming that the primary
MDP device is the one with a specific alias seems useable because the
alternative is to add a property to the device tree which doesn't
actually represent any facet of hardware (i.e., this being the primary
MDP device is a software decision). In other words, this solution is
equally as arbitrary, but at least it doesn't add a property to a
device node where said property is unrelated to the hardware present.
Signed-off-by: Eizan Miyamoto <redacted>
Other than the above,
Reviewed-by: Enric Balletbo i Serra <redacted>
quoted hunk
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
2 files changed, 64 insertions(+), 28 deletions(-)
... Instead of depending on the presence of a mediatek,vpu property in
Looks like there is something missing in the commit message?
That line is a continuation of the commit message header, I.e., it's
meant to read:
"use mdp-rdma0 alias to point to MDP master Instead of depending on
the presence of a mediatek,vpu property in the device node"
Eizan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
quoted hunk
... Instead of depending on the presence of a mediatek,vpu property
in
the device node.
That property was originally added to link to the vpu node so that
the
mtk_mdp_core driver could pass the right device to
vpu_wdt_reg_handler(). However in a previous patch in this series,
the driver has been modified to search the device tree for that node
instead.
That property was also used to indicate the primary MDP device, so
that
it can be passed to the V4L2 subsystem as well as register it to be
used when setting up queues in the open() callback for the filesystem
device node that is created. In this case, assuming that the primary
MDP device is the one with a specific alias seems useable because the
alternative is to add a property to the device tree which doesn't
actually represent any facet of hardware (i.e., this being the
primary
MDP device is a software decision). In other words, this solution is
equally as arbitrary, but at least it doesn't add a property to a
device node where said property is unrelated to the hardware present.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++--
----
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
2 files changed, 64 insertions(+), 28 deletions(-)
*comp)
mtk_smi_larb_put(comp->larb_dev);
}
-static int mtk_mdp_comp_bind(struct device *dev, struct device
*master, void *data)
+/*
+ * The MDP master device node is identified by the device tree alias
+ * "mdp-rdma0".
+ */
+static bool is_mdp_master(struct device *dev)
+{
+ struct device_node *aliases, *mdp_rdma0_node;
+ const char *mdp_rdma0_path;
+
+ if (!dev->of_node)
+ return false;
+
+ aliases = of_find_node_by_path("/aliases");
+ if (!aliases) {
+ dev_err(dev, "no aliases found for mdp-rdma0");
+ return false;
+ }
+
+ mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
+ if (!mdp_rdma0_path) {
+ dev_err(dev, "get mdp-rdma0 property of /aliases
failed");
+ return false;
+ }
+
+ mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
+ if (!mdp_rdma0_node) {
+ dev_err(dev, "path resolution failed for %s",
mdp_rdma0_path);
+ return false;
+ }
+
Hi Eizan,
"mdp-rdma0" may be not the only one master device node. In fact, there
are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016
If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will have
one more V4L2 video devie node.
mdp_rdma1: rdma@14002000 {
compatible = "mediatek,mt8173-mdp-rdma",
"mediatek,mt8173-mdp";
...
}
We should consider the case that there are more than one "MDP_RDMA"
components.
Regards,
Houlong
quoted hunk
+ return dev->of_node == mdp_rdma0_node;
+}
+
+static int mtk_mdp_comp_bind(struct device *dev, struct device
*master,
+ void *data)
{
struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
struct mtk_mdp_dev *mdp = data;
- struct device_node *vpu_node;
mtk_mdp_register_component(mdp, comp);
- /*
- * If this component has a "mediatek-vpu" property, it is
responsible for
- * notifying the mdp master driver about it so it can be
further initialized
- * later.
- */
- vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
- if (vpu_node) {
+ if (is_mdp_master(dev)) {
int ret;
- mdp->vpu_dev = of_find_device_by_node(vpu_node);
- if (WARN_ON(!mdp->vpu_dev)) {
- dev_err(dev, "vpu pdev failed\n");
- of_node_put(vpu_node);
- }
-
ret = v4l2_device_register(dev, &mdp->v4l2_dev);
if (ret) {
dev_err(dev, "Failed to register v4l2
device\n");
@@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev,
struct device *master, void *da
}
/*
- * presence of the "mediatek,vpu" property in a device
node
- * indicates that it is the primary MDP rdma device and
MDP DMA
- * ops should be handled by its DMA callbacks.
+ * MDP DMA ops will be handled by the DMA callbacks
associated with this
+ * device;
*/
mdp->rdma_dev = dev;
}
On Mon, 2021-08-16 at 11:00 +0800, houlong wei wrote:
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
quoted
... Instead of depending on the presence of a mediatek,vpu property
in
the device node.
That property was originally added to link to the vpu node so that
the
mtk_mdp_core driver could pass the right device to
vpu_wdt_reg_handler(). However in a previous patch in this series,
the driver has been modified to search the device tree for that
node
instead.
That property was also used to indicate the primary MDP device, so
that
it can be passed to the V4L2 subsystem as well as register it to be
used when setting up queues in the open() callback for the
filesystem
device node that is created. In this case, assuming that the
primary
MDP device is the one with a specific alias seems useable because
the
alternative is to add a property to the device tree which doesn't
actually represent any facet of hardware (i.e., this being the
primary
MDP device is a software decision). In other words, this solution
is
equally as arbitrary, but at least it doesn't add a property to a
device node where said property is unrelated to the hardware
present.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++--
----
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
2 files changed, 64 insertions(+), 28 deletions(-)
If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will
have
one more V4L2 video devie node.
mdp_rdma1: rdma@14002000 {
compatible = "mediatek,mt8173-mdp-rdma",
"mediatek,mt8173-mdp";
...
}
We should consider the case that there are more than one "MDP_RDMA"
components.
Regards,
Houlong
quoted
+ return dev->of_node == mdp_rdma0_node;
+}
+
+static int mtk_mdp_comp_bind(struct device *dev, struct device
*master,
+ void *data)
{
struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
struct mtk_mdp_dev *mdp = data;
- struct device_node *vpu_node;
mtk_mdp_register_component(mdp, comp);
- /*
- * If this component has a "mediatek-vpu" property, it is
responsible for
- * notifying the mdp master driver about it so it can be
further initialized
- * later.
- */
- vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
- if (vpu_node) {
+ if (is_mdp_master(dev)) {
int ret;
- mdp->vpu_dev = of_find_device_by_node(vpu_node);
- if (WARN_ON(!mdp->vpu_dev)) {
- dev_err(dev, "vpu pdev failed\n");
- of_node_put(vpu_node);
- }
-
ret = v4l2_device_register(dev, &mdp->v4l2_dev);
if (ret) {
dev_err(dev, "Failed to register v4l2
device\n");
@@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device
*dev,
struct device *master, void *da
}
/*
- * presence of the "mediatek,vpu" property in a device
node
- * indicates that it is the primary MDP rdma device and
MDP DMA
- * ops should be handled by its DMA callbacks.
+ * MDP DMA ops will be handled by the DMA callbacks
associated with this
+ * device;
*/
mdp->rdma_dev = dev;
}
*data)
static int mtk_mdp_master_bind(struct device *dev)
{
- int status;
struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
+ struct device_node *vpu_node;
+ int status;
status = component_bind_all(dev, mdp);
if (status) {
@@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device
*dev)
goto err_component_bind_all;
}
- if (mdp->vpu_dev) {
- int ret = vpu_wdt_reg_handler(mdp->vpu_dev,
mtk_mdp_reset_handler, mdp,
- VPU_RST_MDP);
- if (ret) {
- dev_err(dev, "Failed to register reset
handler\n");
- goto err_wdt_reg;
- }
- } else {
- dev_err(dev, "no vpu_dev found\n");
+ if (mdp->rdma_dev == NULL) {
+ dev_err(dev, "Primary MDP device not found");
+ status = -ENODEV;
+ goto err_component_bind_all;
+ }
+
+ vpu_node = of_find_node_by_name(NULL, "vpu");
+ if (!vpu_node) {
+ dev_err(dev, "unable to find vpu node");
+ status = -ENODEV;
+ goto err_wdt_reg;
+ }
Hi Eizan,
Is your removing "mediatek,vpu" necessary for this series "Refactor MTK
MDP driver into core/components" ?
In some MediaTek projects (not upstream yet), the device-tree node name
"vpu" may be changed to something like "vpu0", "vpu1" or other name,
which depends on the implementation of "mtk-vpu" driver. We can specify
the different phandle to "mediatek,vpu" in .dtsi file. If we use
of_find_node_by_name() to get the vpu_node, we have to modify the name
string in different project.
If the answer of my previous questions is "No", I prefer to slow down
the modification of removing "mediatek,vpu".
Sorry for late reply.
Regards,
Houlong
quoted
+
+ mdp->vpu_dev = of_find_device_by_node(vpu_node);
+ if (!mdp->vpu_dev) {
+ dev_err(dev, "unable to find vpu device");
+ status = -ENODEV;
+ goto err_wdt_reg;
+ }
+
+ status = vpu_wdt_reg_handler(mdp->vpu_dev,
mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
+ if (status) {
+ dev_err(dev, "Failed to register reset handler\n");
+ goto err_wdt_reg;
}
status = mtk_mdp_register_m2m_device(mdp);
--
2.32.0.554.ge1b32706d8-goog
Hi Houlong,
Thank you for your review on this series, it is much appreciated.
On Mon, Aug 16, 2021 at 2:53 PM houlong wei [off-list ref] wrote:
Is your removing "mediatek,vpu" necessary for this series "Refactor MTK
MDP driver into core/components" ?
Removing it is not strictly necessary for the series to function, I
will re-upload the series and omit the following changes:
- [PATCH v6 8/9] dts: mtk-mdp: remove mediatek,vpu property from
primary MDP device
- [PATCH v6 9/9] dt-bindings: mediatek: remove vpu requirement from mtk-mdp
Do please let me know if you meant something different.
Thanks,
Eizan
In some MediaTek projects (not upstream yet), the device-tree node name
"vpu" may be changed to something like "vpu0", "vpu1" or other name,
which depends on the implementation of "mtk-vpu" driver. We can specify
the different phandle to "mediatek,vpu" in .dtsi file. If we use
of_find_node_by_name() to get the vpu_node, we have to modify the name
string in different project.
If the answer of my previous questions is "No", I prefer to slow down
the modification of removing "mediatek,vpu".
Sorry for late reply.
Regards,
Houlong
On Wed, 2021-08-18 at 15:50 +0800, Eizan Miyamoto wrote:
Hi Houlong,
Thank you for your review on this series, it is much appreciated.
On Mon, Aug 16, 2021 at 2:53 PM houlong wei <houlong.wei@mediatek.com
quoted
wrote:
Is your removing "mediatek,vpu" necessary for this series "Refactor
MTK
MDP driver into core/components" ?
Removing it is not strictly necessary for the series to function, I
will re-upload the series and omit the following changes:
- [PATCH v6 8/9] dts: mtk-mdp: remove mediatek,vpu property from
primary MDP device
- [PATCH v6 9/9] dt-bindings: mediatek: remove vpu requirement from
mtk-mdp
Do please let me know if you meant something different.
Thanks,
Eizan
Hi Eizan,
Thanks for your help and effort. It's exactly what I expressed.
Regards,
Houlong
quoted
In some MediaTek projects (not upstream yet), the device-tree node
name
"vpu" may be changed to something like "vpu0", "vpu1" or other
name,
which depends on the implementation of "mtk-vpu" driver. We can
specify
the different phandle to "mediatek,vpu" in .dtsi file. If we use
of_find_node_by_name() to get the vpu_node, we have to modify the
name
string in different project.
If the answer of my previous questions is "No", I prefer to slow
down
the modification of removing "mediatek,vpu".
Sorry for late reply.
Regards,
Houlong
Hi Houlong,
On Mon, Aug 16, 2021 at 1:00 PM houlong wei [off-list ref] wrote:
Hi Eizan,
"mdp-rdma0" may be not the only one master device node. In fact, there
are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016
If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will have
one more V4L2 video devie node.
mdp_rdma1: rdma@14002000 {
compatible = "mediatek,mt8173-mdp-rdma",
"mediatek,mt8173-mdp";
...
}
We should consider the case that there are more than one "MDP_RDMA"
components.
Would it be okay with you if we added support for multiple MDP master
device nodes in follow-up changes? My rationale is this:
- As far as I can tell, the mediatek integration with V4L2 currently
only handles a single MDP master device node. It's not clear to me the
scope of changes that will be needed to make things work properly with
multiple nodes.
- The patch series makes video decode work (admittedly, in light of
your comments not optimally) upstream, which is better than not
landing these changes at all.
I'd like to say that I'm very open to (and excited about) discussing
further work to support multiple MDP master nodes, perhaps we can
work together on this so I can understand what needs to be done.
Please let me know your thoughts,
Eizan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Eizan,
Firstly, about how to determine the master mdp driver, we also can
judge the component type. The component type can be gotten by calling
of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is
the master driver. No matter it is mdp_rdma0 or mdp_rdma1.
Secondly, about supporting the multiple MDP master device nodes, you
can try my advice in my previous comment after the completion of this
series of patches.
Thanks a lot.
Regards,
Houlong
On Wed, 2021-08-18 at 15:43 +0800, Eizan Miyamoto wrote:
Hi Houlong,
On Mon, Aug 16, 2021 at 1:00 PM houlong wei <houlong.wei@mediatek.com
quoted
wrote:
Hi Eizan,
"mdp-rdma0" may be not the only one master device node. In fact,
there
are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link.
If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will
have
one more V4L2 video devie node.
mdp_rdma1: rdma@14002000 {
compatible = "mediatek,mt8173-mdp-rdma",
"mediatek,mt8173-mdp";
...
}
We should consider the case that there are more than one "MDP_RDMA"
components.
Would it be okay with you if we added support for multiple MDP master
device nodes in follow-up changes? My rationale is this:
- As far as I can tell, the mediatek integration with V4L2 currently
only handles a single MDP master device node. It's not clear to me
the
scope of changes that will be needed to make things work properly
with
multiple nodes.
- The patch series makes video decode work (admittedly, in light of
your comments not optimally) upstream, which is better than not
landing these changes at all.
I'd like to say that I'm very open to (and excited about) discussing
further work to support multiple MDP master nodes, perhaps we can
work together on this so I can understand what needs to be done.
Please let me know your thoughts,
Eizan
It is no longer used by the mediatek MDP driver.
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 -
1 file changed, 1 deletion(-)
It is no longer needed by the mtk-mdp driver
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
Documentation/devicetree/bindings/media/mediatek-mdp.txt | 3 ---
1 file changed, 3 deletions(-)
@@ -4,8 +4,6 @@ Media Data Path is used for scaling and color space conversion. Required properties (controller node): - compatible: "mediatek,mt8173-mdp"-- mediatek,vpu: the node of video processor unit, see- Documentation/devicetree/bindings/media/mediatek-vpu.txt for details. Required properties (all function blocks, child node): - compatible: Should be one of
@@ -4,8 +4,6 @@ Media Data Path is used for scaling and color space conversion. Required properties (controller node): - compatible: "mediatek,mt8173-mdp"-- mediatek,vpu: the node of video processor unit, see- Documentation/devicetree/bindings/media/mediatek-vpu.txt for details. Required properties (all function blocks, child node): - compatible: Should be one of
From: Rob Herring <robh@kernel.org> Date: 2021-08-06 21:47:34
On Mon, 02 Aug 2021 22:12:15 +1000, Eizan Miyamoto wrote:
It is no longer needed by the mtk-mdp driver
Signed-off-by: Eizan Miyamoto <redacted>
---
(no changes since v1)
Documentation/devicetree/bindings/media/mediatek-mdp.txt | 3 ---
1 file changed, 3 deletions(-)
The series have also been validated on top of 5.14-rc4 and linux-next
on an Acer Chromebook R 13 without observing any problems and running
some video decoding tests, so, for the full series.
Tested-by: Enric Balletbo i Serra <redacted>
Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
actually loading it from mmsys into two changs to make review easier.
- Update devicetree bindings to reflect no longer needing the
mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.
Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6
Changes in v4:
- rebase and test on 5.13
- don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name
Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
mediatek,vpu property inside the mdp_rdma0 device node.
Eizan Miyamoto (9):
mtk-mdp: propagate errors from clock_on
mtk-mdp: add driver to probe mdp components
mtk-mdp: use pm_runtime in MDP component driver
media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on
mtk-mdp: make mdp driver to be loadable by platform_device_register*()
soc: mediatek: mmsys: instantiate mdp virtual device from mmsys
media: mtk-mdp: use mdp-rdma0 alias to point to MDP master
dts: mtk-mdp: remove mediatek,vpu property from primary MDP device
dt-bindings: mediatek: remove vpu requirement from mtk-mdp
.../bindings/media/mediatek-mdp.txt | 3 -
arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 -
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 268 +++++++++++++++--
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 34 +--
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 282 ++++++++++++------
drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 3 +
drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 4 +-
drivers/soc/mediatek/mtk-mmsys.c | 20 +-
8 files changed, 470 insertions(+), 145 deletions(-)
--
2.32.0.554.ge1b32706d8-goog