| Message ID | 20241212-qcom-video-iris-v9-27-e8c2c6bd4041@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Qualcomm iris video decoder driver | expand |
Em Thu, 12 Dec 2024 17:21:49 +0530
Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu:
> + .dma_mask = GENMASK(31, 29) - 1,
Setting a mask to GENMASK() - 1 sounds weird. Is it really what you want?
I so, why?
Thanks,
Mauro
On 12/23/2024 4:00 PM, Mauro Carvalho Chehab wrote: > Em Thu, 12 Dec 2024 17:21:49 +0530 > Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu: > >> + .dma_mask = GENMASK(31, 29) - 1, > > Setting a mask to GENMASK() - 1 sounds weird. Is it really what you want? > I so, why? > Hi Mauro, the value of this dma mask should be 0xe0000000 -1. The background for the same is, 0xe0000000 onward memory space is allocated for IO register space so we are restricting the driver buffer allocations to 0xe0000000 - 1. Based on the comments received in the past, we are using GENMASK to generate 0xe0000000. Does this answer your query or I missed something? Thanks, Dikshita > Thanks, > Mauro
On 1/8/2025 8:22 PM, Mauro Carvalho Chehab wrote: > Em Wed, 8 Jan 2025 16:42:03 +0530 > Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu: > >> On 1/8/2025 4:13 PM, Hans Verkuil wrote: >>> On 1/8/25 11:21, Dikshita Agarwal wrote: >>>> >>>> >>>> On 1/8/2025 2:25 PM, Hans Verkuil wrote: >>>>> On 08/01/2025 09:51, Dikshita Agarwal wrote: >>>>>> >>>>>> >>>>>> On 1/8/2025 1:17 PM, Hans Verkuil wrote: >>>>>>> On 08/01/2025 08:43, Dikshita Agarwal wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 1/7/2025 7:27 PM, Nicolas Dufresne wrote: >>>>>>>>> Le lundi 23 décembre 2024 à 16:21 +0530, Dikshita Agarwal a écrit : >>>>>>>>>> >>>>>>>>>> On 12/23/2024 4:00 PM, Mauro Carvalho Chehab wrote: >>>>>>>>>>> Em Thu, 12 Dec 2024 17:21:49 +0530 >>>>>>>>>>> Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu: >>>>>>>>>>> >>>>>>>>>>>> + .dma_mask = GENMASK(31, 29) - 1, >>>>>>>>>>> >>>>>>>>>>> Setting a mask to GENMASK() - 1 sounds weird. Is it really what you want? >>>>>>>>>>> I so, why? >>>>>>>>>>> >>>>>>>>>> Hi Mauro, >>>>>>>>>> >>>>>>>>>> the value of this dma mask should be 0xe0000000 -1. >>>>>>>>>> >>>>>>>>>> The background for the same is, 0xe0000000 onward memory space is allocated >>>>>>>>>> for IO register space so we are restricting the driver buffer allocations >>>>>>>>>> to 0xe0000000 - 1. >>>>>>>>>> >>>>>>>>>> Based on the comments received in the past, we are using GENMASK to >>>>>>>>>> generate 0xe0000000. >>>>>>>>>> >>>>>>>>>> Does this answer your query or I missed something? >>>>>>>>> >>>>>>>>> I'm not sure it will do what you want. (0xe0000000 -1) matches ~BIT(29). Perhaps >>>>>>>>> you wanted to use ~0xe0000000. >>>>>>>>> >>>>>>>> value of dma mask is coming as expected with GENMASK(31, 29) - 1 >>>>>>>> >>>>>>>> qcom-iris aa00000.video-codec: dma_mask DFFFFFFF (0xe0000000 -1) >>>>>>> >>>>>>> Isn't this just the equivalent of GENMASK(28, 0)? Can't you use that? >>>>> >>>>> Too early in the morning, this suggestion was clearly wrong. >>>>> >>>>>>> >>>>>>> It's much easier to understand than GENMASK()-1. >>>>>> >>>>>> Sure, I can use either ~GENMASK(29, 29) or ~BIT(29), >>>>> >>>>> ~BIT(29). >>>>> >>>>> It's really weird to just disable a single bit, so I think some comments >>>>> explaining why this mask is needed would be good (if there aren't comments >>>>> already). >>>>> >>>> I tested this some more, and seems ~BIT(29) doesn't work, as its still >>>> conflicting with the register space. >>> >>> Odd, perhaps a 64 vs 32 bit issue? >>> >>>> Correct value would be GENMASK(31,30) + GENMASK(28,0) to set the exact bits >>>> to get the desired value i.e 0xe0000000 -1 >>> Honestly, in this case I would prefer to just go with the actual hex value >>> 0xdfffffff together with an explanatory comment. >>> >> We moved to GENMASK way to address comment on previous version, but sure >> can directly use 0xdfffffff with a comment. > > If I understood it right, bits 0-31 can be used, but the hardware has some > issue using bit 29 at the mask. Could you please comment why it can't be > used? > That would not be a correct statement, We don't have issue with using BIT 29 with mask but upper limit of DMA address available to use is 0xdfffffff. Thanks, Dikshita > Btw, as this is a mask, IMO the better would be to document that all bits > except for BIT(29) can be used with something like: > > /* Bit 29 can't be used because ... */ > .dma_mask = GENMASK(31, 0) - BIT(29) > > Thanks, > Mauro
Hi, On 1/9/25 10:43 AM, Dikshita Agarwal wrote: > > > On 1/8/2025 8:22 PM, Mauro Carvalho Chehab wrote: >> Em Wed, 8 Jan 2025 16:42:03 +0530 >> Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu: >> >>> On 1/8/2025 4:13 PM, Hans Verkuil wrote: >>>> On 1/8/25 11:21, Dikshita Agarwal wrote: >>>>> >>>>> >>>>> On 1/8/2025 2:25 PM, Hans Verkuil wrote: >>>>>> On 08/01/2025 09:51, Dikshita Agarwal wrote: >>>>>>> >>>>>>> >>>>>>> On 1/8/2025 1:17 PM, Hans Verkuil wrote: >>>>>>>> On 08/01/2025 08:43, Dikshita Agarwal wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 1/7/2025 7:27 PM, Nicolas Dufresne wrote: >>>>>>>>>> Le lundi 23 décembre 2024 à 16:21 +0530, Dikshita Agarwal a écrit : >>>>>>>>>>> >>>>>>>>>>> On 12/23/2024 4:00 PM, Mauro Carvalho Chehab wrote: >>>>>>>>>>>> Em Thu, 12 Dec 2024 17:21:49 +0530 >>>>>>>>>>>> Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu: >>>>>>>>>>>> >>>>>>>>>>>>> + .dma_mask = GENMASK(31, 29) - 1, >>>>>>>>>>>> >>>>>>>>>>>> Setting a mask to GENMASK() - 1 sounds weird. Is it really what you want? >>>>>>>>>>>> I so, why? >>>>>>>>>>>> >>>>>>>>>>> Hi Mauro, >>>>>>>>>>> >>>>>>>>>>> the value of this dma mask should be 0xe0000000 -1. >>>>>>>>>>> >>>>>>>>>>> The background for the same is, 0xe0000000 onward memory space is allocated >>>>>>>>>>> for IO register space so we are restricting the driver buffer allocations >>>>>>>>>>> to 0xe0000000 - 1. >>>>>>>>>>> >>>>>>>>>>> Based on the comments received in the past, we are using GENMASK to >>>>>>>>>>> generate 0xe0000000. >>>>>>>>>>> >>>>>>>>>>> Does this answer your query or I missed something? >>>>>>>>>> >>>>>>>>>> I'm not sure it will do what you want. (0xe0000000 -1) matches ~BIT(29). Perhaps >>>>>>>>>> you wanted to use ~0xe0000000. >>>>>>>>>> >>>>>>>>> value of dma mask is coming as expected with GENMASK(31, 29) - 1 >>>>>>>>> >>>>>>>>> qcom-iris aa00000.video-codec: dma_mask DFFFFFFF (0xe0000000 -1) >>>>>>>> >>>>>>>> Isn't this just the equivalent of GENMASK(28, 0)? Can't you use that? >>>>>> >>>>>> Too early in the morning, this suggestion was clearly wrong. >>>>>> >>>>>>>> >>>>>>>> It's much easier to understand than GENMASK()-1. >>>>>>> >>>>>>> Sure, I can use either ~GENMASK(29, 29) or ~BIT(29), >>>>>> >>>>>> ~BIT(29). >>>>>> >>>>>> It's really weird to just disable a single bit, so I think some comments >>>>>> explaining why this mask is needed would be good (if there aren't comments >>>>>> already). >>>>>> >>>>> I tested this some more, and seems ~BIT(29) doesn't work, as its still >>>>> conflicting with the register space. >>>> >>>> Odd, perhaps a 64 vs 32 bit issue? >>>> >>>>> Correct value would be GENMASK(31,30) + GENMASK(28,0) to set the exact bits >>>>> to get the desired value i.e 0xe0000000 -1 >>>> Honestly, in this case I would prefer to just go with the actual hex value >>>> 0xdfffffff together with an explanatory comment. >>>> >>> We moved to GENMASK way to address comment on previous version, but sure >>> can directly use 0xdfffffff with a comment. >> >> If I understood it right, bits 0-31 can be used, but the hardware has some >> issue using bit 29 at the mask. Could you please comment why it can't be >> used? >> > That would not be a correct statement, We don't have issue with using BIT > 29 with mask but upper limit of DMA address available to use is 0xdfffffff. I'd keep the original representation of the DMA address mask. This is not an register to represent it via GENMASK. It is used by the driver to set the hardware limitation on the upper bound of DMA address range. .dma_mask = 0xe0000000 - 1 because we set it through dma_set_mask_and_coherent() which expects a mask we subtract 1. ~Stan
On 09/01/2025 10:49, Stanimir Varbanov wrote: > Hi, > > On 1/9/25 10:43 AM, Dikshita Agarwal wrote: >> >> >> On 1/8/2025 8:22 PM, Mauro Carvalho Chehab wrote: >>> Em Wed, 8 Jan 2025 16:42:03 +0530 >>> Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu: >>> >>>> On 1/8/2025 4:13 PM, Hans Verkuil wrote: >>>>> On 1/8/25 11:21, Dikshita Agarwal wrote: >>>>>> >>>>>> >>>>>> On 1/8/2025 2:25 PM, Hans Verkuil wrote: >>>>>>> On 08/01/2025 09:51, Dikshita Agarwal wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 1/8/2025 1:17 PM, Hans Verkuil wrote: >>>>>>>>> On 08/01/2025 08:43, Dikshita Agarwal wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 1/7/2025 7:27 PM, Nicolas Dufresne wrote: >>>>>>>>>>> Le lundi 23 décembre 2024 à 16:21 +0530, Dikshita Agarwal a écrit : >>>>>>>>>>>> >>>>>>>>>>>> On 12/23/2024 4:00 PM, Mauro Carvalho Chehab wrote: >>>>>>>>>>>>> Em Thu, 12 Dec 2024 17:21:49 +0530 >>>>>>>>>>>>> Dikshita Agarwal <quic_dikshita@quicinc.com> escreveu: >>>>>>>>>>>>> >>>>>>>>>>>>>> + .dma_mask = GENMASK(31, 29) - 1, >>>>>>>>>>>>> >>>>>>>>>>>>> Setting a mask to GENMASK() - 1 sounds weird. Is it really what you want? >>>>>>>>>>>>> I so, why? >>>>>>>>>>>>> >>>>>>>>>>>> Hi Mauro, >>>>>>>>>>>> >>>>>>>>>>>> the value of this dma mask should be 0xe0000000 -1. >>>>>>>>>>>> >>>>>>>>>>>> The background for the same is, 0xe0000000 onward memory space is allocated >>>>>>>>>>>> for IO register space so we are restricting the driver buffer allocations >>>>>>>>>>>> to 0xe0000000 - 1. >>>>>>>>>>>> >>>>>>>>>>>> Based on the comments received in the past, we are using GENMASK to >>>>>>>>>>>> generate 0xe0000000. >>>>>>>>>>>> >>>>>>>>>>>> Does this answer your query or I missed something? >>>>>>>>>>> >>>>>>>>>>> I'm not sure it will do what you want. (0xe0000000 -1) matches ~BIT(29). Perhaps >>>>>>>>>>> you wanted to use ~0xe0000000. >>>>>>>>>>> >>>>>>>>>> value of dma mask is coming as expected with GENMASK(31, 29) - 1 >>>>>>>>>> >>>>>>>>>> qcom-iris aa00000.video-codec: dma_mask DFFFFFFF (0xe0000000 -1) >>>>>>>>> >>>>>>>>> Isn't this just the equivalent of GENMASK(28, 0)? Can't you use that? >>>>>>> >>>>>>> Too early in the morning, this suggestion was clearly wrong. >>>>>>> >>>>>>>>> >>>>>>>>> It's much easier to understand than GENMASK()-1. >>>>>>>> >>>>>>>> Sure, I can use either ~GENMASK(29, 29) or ~BIT(29), >>>>>>> >>>>>>> ~BIT(29). >>>>>>> >>>>>>> It's really weird to just disable a single bit, so I think some comments >>>>>>> explaining why this mask is needed would be good (if there aren't comments >>>>>>> already). >>>>>>> >>>>>> I tested this some more, and seems ~BIT(29) doesn't work, as its still >>>>>> conflicting with the register space. >>>>> >>>>> Odd, perhaps a 64 vs 32 bit issue? >>>>> >>>>>> Correct value would be GENMASK(31,30) + GENMASK(28,0) to set the exact bits >>>>>> to get the desired value i.e 0xe0000000 -1 >>>>> Honestly, in this case I would prefer to just go with the actual hex value >>>>> 0xdfffffff together with an explanatory comment. >>>>> >>>> We moved to GENMASK way to address comment on previous version, but sure >>>> can directly use 0xdfffffff with a comment. >>> >>> If I understood it right, bits 0-31 can be used, but the hardware has some >>> issue using bit 29 at the mask. Could you please comment why it can't be >>> used? >>> >> That would not be a correct statement, We don't have issue with using BIT >> 29 with mask but upper limit of DMA address available to use is 0xdfffffff. > > I'd keep the original representation of the DMA address mask. This is > not an register to represent it via GENMASK. It is used by the driver to > set the hardware limitation on the upper bound of DMA address range. > > .dma_mask = 0xe0000000 - 1 > > because we set it through dma_set_mask_and_coherent() which expects a > mask we subtract 1. Yes, I agree. I see the venus driver does the same thing, so let's follow that example for consistency. Regards, Hans > > ~Stan > >
On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: > Initialize the platform data and enable video driver probe of SM8250 > SoC. Add a kernel param to select between venus and iris drivers for > platforms supported by both drivers, for ex: SM8250. Why do you want to use a module parameter for this? What would be the default configuration? (Module parameters should generally be avoided.) Why not simply switch to the new driver (and make sure that the new driver is selected if the old one was enabled in the kernel config)? > Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org> # x1e80100 (Dell Looks like something is missing from Stefan's Tested-by tag throughout the series ("Dell XPS13"?) > Reviewed-by: Stefan Schmidt <stefan.schmidt@linaro.org> > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > +static bool prefer_venus = true; > +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); > +module_param(prefer_venus, bool, 0444); > + > +/* list all platforms supported by only iris driver */ > +static const char *const iris_only_platforms[] = { > + "qcom,sm8550-iris", > + NULL, > +}; Surely you don't want to have to add every new platform to two tables (i.e. the id table and again here). > + > +/* list all platforms supported by both venus and iris drivers */ > +static const char *const venus_to_iris_migration[] = { > + "qcom,sm8250-venus", > + NULL, > +}; > + > +static bool video_drv_should_bind(struct device *dev, bool is_iris_driver) > +{ > + if (of_device_compatible_match(dev->of_node, iris_only_platforms)) > + return is_iris_driver; > + > + /* If it is not in the migration list, use venus */ > + if (!of_device_compatible_match(dev->of_node, venus_to_iris_migration)) > + return !is_iris_driver; > + > + return prefer_venus ? !is_iris_driver : is_iris_driver; > +} > + > static int iris_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) > u64 dma_mask; > int ret; > > + if (!video_drv_should_bind(&pdev->dev, true)) > + return -ENODEV; AFAICT nothing is preventing venus from binding even when 'prefer_venus' is false. > + > core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > if (!core) > return -ENOMEM; > @@ -324,6 +355,10 @@ static const struct of_device_id iris_dt_match[] = { > .compatible = "qcom,sm8550-iris", > .data = &sm8550_data, > }, > + { > + .compatible = "qcom,sm8250-venus", > + .data = &sm8250_data, > + }, > { }, > }; > MODULE_DEVICE_TABLE(of, iris_dt_match); Johan
Hi Johan, On 1/9/2025 8:41 PM, Johan Hovold wrote: > On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: >> Initialize the platform data and enable video driver probe of SM8250 >> SoC. Add a kernel param to select between venus and iris drivers for >> platforms supported by both drivers, for ex: SM8250. > > Why do you want to use a module parameter for this? What would be the > default configuration? (Module parameters should generally be avoided.) This was discussed during v4 [1] and implemented as per suggestion [1] https://lore.kernel.org/linux-media/eea14133-2152-37bb-e2ff-fcc7ed4c47f5@quicinc.com/ > > Why not simply switch to the new driver (and make sure that the new > driver is selected if the old one was enabled in the kernel config)? Its about the platform in migration i.e sm8250. Since new driver is not yet feature parity with old driver, choice is provided to client if it wants to use the new driver (default being old driver for sm8250) Regards, Vikash >> Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org> # x1e80100 (Dell > > Looks like something is missing from Stefan's Tested-by tag throughout > the series ("Dell XPS13"?) > >> Reviewed-by: Stefan Schmidt <stefan.schmidt@linaro.org> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > >> +static bool prefer_venus = true; >> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); >> +module_param(prefer_venus, bool, 0444); >> + >> +/* list all platforms supported by only iris driver */ >> +static const char *const iris_only_platforms[] = { >> + "qcom,sm8550-iris", >> + NULL, >> +}; > > Surely you don't want to have to add every new platform to two tables > (i.e. the id table and again here). > >> + >> +/* list all platforms supported by both venus and iris drivers */ >> +static const char *const venus_to_iris_migration[] = { >> + "qcom,sm8250-venus", >> + NULL, >> +}; >> + >> +static bool video_drv_should_bind(struct device *dev, bool is_iris_driver) >> +{ >> + if (of_device_compatible_match(dev->of_node, iris_only_platforms)) >> + return is_iris_driver; >> + >> + /* If it is not in the migration list, use venus */ >> + if (!of_device_compatible_match(dev->of_node, venus_to_iris_migration)) >> + return !is_iris_driver; >> + >> + return prefer_venus ? !is_iris_driver : is_iris_driver; >> +} >> + >> static int iris_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) >> u64 dma_mask; >> int ret; >> >> + if (!video_drv_should_bind(&pdev->dev, true)) >> + return -ENODEV; > > AFAICT nothing is preventing venus from binding even when 'prefer_venus' > is false. > >> + >> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); >> if (!core) >> return -ENOMEM; >> @@ -324,6 +355,10 @@ static const struct of_device_id iris_dt_match[] = { >> .compatible = "qcom,sm8550-iris", >> .data = &sm8550_data, >> }, >> + { >> + .compatible = "qcom,sm8250-venus", >> + .data = &sm8250_data, >> + }, >> { }, >> }; >> MODULE_DEVICE_TABLE(of, iris_dt_match); > > Johan
On Thu, Jan 09, 2025 at 04:11:04PM +0100, Johan Hovold wrote: > On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: > > Initialize the platform data and enable video driver probe of SM8250 > > SoC. Add a kernel param to select between venus and iris drivers for > > platforms supported by both drivers, for ex: SM8250. > > Why do you want to use a module parameter for this? What would be the > default configuration? (Module parameters should generally be avoided.) > > Why not simply switch to the new driver (and make sure that the new > driver is selected if the old one was enabled in the kernel config)? Because the new driver doesn't yet have feature parity with the venus driver. So it was agreed that developers provide upgrade path to allow users to gradually test and switch to the new driver. When the feature parity is achieved, the plan is to switch default to point to the Iris driver, then after a few releases start removing platforms from Venus. > > Tested-by: Stefan Schmidt <stefan.schmidt@linaro.org> # x1e80100 (Dell > > Looks like something is missing from Stefan's Tested-by tag throughout > the series ("Dell XPS13"?) > > > Reviewed-by: Stefan Schmidt <stefan.schmidt@linaro.org> > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > > > +static bool prefer_venus = true; > > +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); > > +module_param(prefer_venus, bool, 0444); > > + > > +/* list all platforms supported by only iris driver */ > > +static const char *const iris_only_platforms[] = { > > + "qcom,sm8550-iris", > > + NULL, > > +}; > > Surely you don't want to have to add every new platform to two tables > (i.e. the id table and again here). I'd agree here, this list should go. We should only list platforms under the migration. > > > + > > +/* list all platforms supported by both venus and iris drivers */ > > +static const char *const venus_to_iris_migration[] = { > > + "qcom,sm8250-venus", > > + NULL, > > +}; > > + > > +static bool video_drv_should_bind(struct device *dev, bool is_iris_driver) The name should follow other names in the driver. `video_drv_should_bind` doesn't have a common prefix. Also export it and use it from the venus driver if Iris is enabled. > > +{ > > + if (of_device_compatible_match(dev->of_node, iris_only_platforms)) > > + return is_iris_driver; > > + > > + /* If it is not in the migration list, use venus */ > > + if (!of_device_compatible_match(dev->of_node, venus_to_iris_migration)) > > + return !is_iris_driver; > > + > > + return prefer_venus ? !is_iris_driver : is_iris_driver; > > +} > > + > > static int iris_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) > > u64 dma_mask; > > int ret; > > > > + if (!video_drv_should_bind(&pdev->dev, true)) > > + return -ENODEV; > > AFAICT nothing is preventing venus from binding even when 'prefer_venus' > is false. > > > + > > core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > > if (!core) > > return -ENOMEM; > > @@ -324,6 +355,10 @@ static const struct of_device_id iris_dt_match[] = { > > .compatible = "qcom,sm8550-iris", > > .data = &sm8550_data, > > }, > > + { > > + .compatible = "qcom,sm8250-venus", > > + .data = &sm8250_data, > > + }, > > { }, > > }; > > MODULE_DEVICE_TABLE(of, iris_dt_match); > > Johan
On Thu, Jan 09, 2025 at 11:18:29PM +0530, Vikash Garodia wrote: > On 1/9/2025 8:41 PM, Johan Hovold wrote: > > On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: > >> Initialize the platform data and enable video driver probe of SM8250 > >> SoC. Add a kernel param to select between venus and iris drivers for > >> platforms supported by both drivers, for ex: SM8250. > > > > Why do you want to use a module parameter for this? What would be the > > default configuration? (Module parameters should generally be avoided.) > This was discussed during v4 [1] and implemented as per suggestion > > [1] > https://lore.kernel.org/linux-media/eea14133-2152-37bb-e2ff-fcc7ed4c47f5@quicinc.com/ First, the background and motivation for this still needs to go in the commit message (and be mentioned in the cover letter). Second, what you implemented here is not even equivalent to what was done in the mdm drm driver since that module parameter is honoured by both drivers so that at most one driver tries to bind to the platform device. With this patch as it stands, which driver ends up binding depends on things like link order and what driver has been built a module, etc. (as I pointed out below). > > Why not simply switch to the new driver (and make sure that the new > > driver is selected if the old one was enabled in the kernel config)? > Its about the platform in migration i.e sm8250. Since new driver is not yet > feature parity with old driver, choice is provided to client if it wants to use > the new driver (default being old driver for sm8250) This should be described in the commit message, along with details on what the delta is so that the reasoning can be evaluated. And I'm still not sure using a module parameter for this is the right thing to do as it is generally something that should be avoided. > >> static int iris_probe(struct platform_device *pdev) > >> { > >> struct device *dev = &pdev->dev; > >> @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) > >> u64 dma_mask; > >> int ret; > >> > >> + if (!video_drv_should_bind(&pdev->dev, true)) > >> + return -ENODEV; > > > > AFAICT nothing is preventing venus from binding even when 'prefer_venus' > > is false. > > > >> + > >> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > >> if (!core) > >> return -ENOMEM; Johan
On 1/10/2025 7:58 PM, Johan Hovold wrote: > On Thu, Jan 09, 2025 at 11:18:29PM +0530, Vikash Garodia wrote: >> On 1/9/2025 8:41 PM, Johan Hovold wrote: >>> On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: >>>> Initialize the platform data and enable video driver probe of SM8250 >>>> SoC. Add a kernel param to select between venus and iris drivers for >>>> platforms supported by both drivers, for ex: SM8250. >>> >>> Why do you want to use a module parameter for this? What would be the >>> default configuration? (Module parameters should generally be avoided.) > >> This was discussed during v4 [1] and implemented as per suggestion >> >> [1] >> https://lore.kernel.org/linux-media/eea14133-2152-37bb-e2ff-fcc7ed4c47f5@quicinc.com/ > > First, the background and motivation for this still needs to go in the > commit message (and be mentioned in the cover letter). > > Second, what you implemented here is not even equivalent to what was > done in the mdm drm driver since that module parameter is honoured by > both drivers so that at most one driver tries to bind to the platform > device. > > With this patch as it stands, which driver ends up binding depends on > things like link order and what driver has been built a module, etc. (as > I pointed out below). > >>> Why not simply switch to the new driver (and make sure that the new >>> driver is selected if the old one was enabled in the kernel config)? > >> Its about the platform in migration i.e sm8250. Since new driver is not yet >> feature parity with old driver, choice is provided to client if it wants to use >> the new driver (default being old driver for sm8250) > > This should be described in the commit message, along with details on > what the delta is so that the reasoning can be evaluated. > > And I'm still not sure using a module parameter for this is the right > thing to do as it is generally something that should be avoided. > I understand your concern of using module params. I will modify it to rely on Kconfig to select the driver (suggested by Hans) instead of module param. something like: config VIDEO_QCOM_IRIS tristate "Qualcomm iris V4L2 decoder driver" ... depends on VIDEO_QCOM_VENUS=n || COMPILE_TEST Thanks, Dikshita >>>> static int iris_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) >>>> u64 dma_mask; >>>> int ret; >>>> >>>> + if (!video_drv_should_bind(&pdev->dev, true)) >>>> + return -ENODEV; >>> >>> AFAICT nothing is preventing venus from binding even when 'prefer_venus' >>> is false. >>> >>>> + >>>> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); >>>> if (!core) >>>> return -ENOMEM; > > Johan
On 10 January 2025 19:30:30 EET, Dikshita Agarwal <quic_dikshita@quicinc.com> wrote: > > >On 1/10/2025 7:58 PM, Johan Hovold wrote: >> On Thu, Jan 09, 2025 at 11:18:29PM +0530, Vikash Garodia wrote: >>> On 1/9/2025 8:41 PM, Johan Hovold wrote: >>>> On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: >>>>> Initialize the platform data and enable video driver probe of SM8250 >>>>> SoC. Add a kernel param to select between venus and iris drivers for >>>>> platforms supported by both drivers, for ex: SM8250. >>>> >>>> Why do you want to use a module parameter for this? What would be the >>>> default configuration? (Module parameters should generally be avoided.) >> >>> This was discussed during v4 [1] and implemented as per suggestion >>> >>> [1] >>> https://lore.kernel.org/linux-media/eea14133-2152-37bb-e2ff-fcc7ed4c47f5@quicinc.com/ >> >> First, the background and motivation for this still needs to go in the >> commit message (and be mentioned in the cover letter). >> >> Second, what you implemented here is not even equivalent to what was >> done in the mdm drm driver since that module parameter is honoured by >> both drivers so that at most one driver tries to bind to the platform >> device. >> >> With this patch as it stands, which driver ends up binding depends on >> things like link order and what driver has been built a module, etc. (as >> I pointed out below). >> >>>> Why not simply switch to the new driver (and make sure that the new >>>> driver is selected if the old one was enabled in the kernel config)? >> >>> Its about the platform in migration i.e sm8250. Since new driver is not yet >>> feature parity with old driver, choice is provided to client if it wants to use >>> the new driver (default being old driver for sm8250) >> >> This should be described in the commit message, along with details on >> what the delta is so that the reasoning can be evaluated. >> >> And I'm still not sure using a module parameter for this is the right >> thing to do as it is generally something that should be avoided. >> >I understand your concern of using module params. >I will modify it to rely on Kconfig to select the driver (suggested by >Hans) instead of module param. Please don't. This makes it impossible to perform side-by-side comparison. Also as venus and iris drivers are not completely equivalent wrt supported platforms, distributions will have to select whether to disable support for older platforms or for new platforms: Kconfig dependency will make it impossible to enable support for both kinds. >something like: >config VIDEO_QCOM_IRIS > tristate "Qualcomm iris V4L2 decoder driver" > ... > depends on VIDEO_QCOM_VENUS=n || COMPILE_TEST > >Thanks, >Dikshita >>>>> static int iris_probe(struct platform_device *pdev) >>>>> { >>>>> struct device *dev = &pdev->dev; >>>>> @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) >>>>> u64 dma_mask; >>>>> int ret; >>>>> >>>>> + if (!video_drv_should_bind(&pdev->dev, true)) >>>>> + return -ENODEV; >>>> >>>> AFAICT nothing is preventing venus from binding even when 'prefer_venus' >>>> is false. >>>> >>>>> + >>>>> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); >>>>> if (!core) >>>>> return -ENOMEM; >> >> Johan
On 10/01/2025 19:01, Dmitry Baryshkov wrote: > On 10 January 2025 19:30:30 EET, Dikshita Agarwal <quic_dikshita@quicinc.com> wrote: >> >> >> On 1/10/2025 7:58 PM, Johan Hovold wrote: >>> On Thu, Jan 09, 2025 at 11:18:29PM +0530, Vikash Garodia wrote: >>>> On 1/9/2025 8:41 PM, Johan Hovold wrote: >>>>> On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: >>>>>> Initialize the platform data and enable video driver probe of SM8250 >>>>>> SoC. Add a kernel param to select between venus and iris drivers for >>>>>> platforms supported by both drivers, for ex: SM8250. >>>>> >>>>> Why do you want to use a module parameter for this? What would be the >>>>> default configuration? (Module parameters should generally be avoided.) >>> >>>> This was discussed during v4 [1] and implemented as per suggestion >>>> >>>> [1] >>>> https://lore.kernel.org/linux-media/eea14133-2152-37bb-e2ff-fcc7ed4c47f5@quicinc.com/ >>> >>> First, the background and motivation for this still needs to go in the >>> commit message (and be mentioned in the cover letter). >>> >>> Second, what you implemented here is not even equivalent to what was >>> done in the mdm drm driver since that module parameter is honoured by >>> both drivers so that at most one driver tries to bind to the platform >>> device. >>> >>> With this patch as it stands, which driver ends up binding depends on >>> things like link order and what driver has been built a module, etc. (as >>> I pointed out below). >>> >>>>> Why not simply switch to the new driver (and make sure that the new >>>>> driver is selected if the old one was enabled in the kernel config)? >>> >>>> Its about the platform in migration i.e sm8250. Since new driver is not yet >>>> feature parity with old driver, choice is provided to client if it wants to use >>>> the new driver (default being old driver for sm8250) >>> >>> This should be described in the commit message, along with details on >>> what the delta is so that the reasoning can be evaluated. >>> >>> And I'm still not sure using a module parameter for this is the right >>> thing to do as it is generally something that should be avoided. >>> >> I understand your concern of using module params. >> I will modify it to rely on Kconfig to select the driver (suggested by >> Hans) instead of module param. > > Please don't. This makes it impossible to perform side-by-side comparison. Also as venus and iris drivers are not completely equivalent wrt supported platforms, distributions will have to select whether to disable support for older platforms or for new platforms: Kconfig dependency will make it impossible to enable support for both kinds. An alternative is that the module option is placed under #if defined(CONFIG_VIDEO_QCOM_IRIS) && defined(CONFIG_VIDEO_QCOM_VENUS) So it only activates if both drivers are compiled. But the fact that both drivers can work for the same hardware is something that must be clearly documented. Probably in a comment block before this module option. Possibly also in the Kconfigs for the IRIS and VENUS drivers. Things that are unusual require explanation, so in this case I'd like to see: 1) Why are there two drivers? 2) Why allow runtime-selection of which driver to use? (E.g. side-by-side comparison) 3) Which hardware supports only venus, only iris, or both? 4) What is the road forward? (I assume that venus is removed once feature parity is reached?) Regards, Hans > >> something like: >> config VIDEO_QCOM_IRIS >> tristate "Qualcomm iris V4L2 decoder driver" >> ... >> depends on VIDEO_QCOM_VENUS=n || COMPILE_TEST >> >> Thanks, >> Dikshita >>>>>> static int iris_probe(struct platform_device *pdev) >>>>>> { >>>>>> struct device *dev = &pdev->dev; >>>>>> @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) >>>>>> u64 dma_mask; >>>>>> int ret; >>>>>> >>>>>> + if (!video_drv_should_bind(&pdev->dev, true)) >>>>>> + return -ENODEV; >>>>> >>>>> AFAICT nothing is preventing venus from binding even when 'prefer_venus' >>>>> is false. >>>>> >>>>>> + >>>>>> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); >>>>>> if (!core) >>>>>> return -ENOMEM; >>> >>> Johan >
diff --git a/drivers/media/platform/qcom/iris/Makefile b/drivers/media/platform/qcom/iris/Makefile index ca31db847273..a746681e03cd 100644 --- a/drivers/media/platform/qcom/iris/Makefile +++ b/drivers/media/platform/qcom/iris/Makefile @@ -9,6 +9,7 @@ iris-objs += iris_buffer.o \ iris_hfi_gen2_packet.o \ iris_hfi_gen2_response.o \ iris_hfi_queue.o \ + iris_platform_sm8250.o \ iris_platform_sm8550.o \ iris_power.o \ iris_probe.o \ diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h index 189dd081ad0a..af24ce4fc417 100644 --- a/drivers/media/platform/qcom/iris/iris_platform_common.h +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h @@ -34,6 +34,7 @@ enum pipe_type { }; extern struct iris_platform_data sm8550_data; +extern struct iris_platform_data sm8250_data; enum platform_clk_type { IRIS_AXI_CLK, diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8250.c b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c new file mode 100644 index 000000000000..9ef2fcc1a0fd --- /dev/null +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include "iris_core.h" +#include "iris_ctrls.h" +#include "iris_platform_common.h" +#include "iris_resources.h" +#include "iris_hfi_gen1.h" +#include "iris_hfi_gen1_defines.h" +#include "iris_vpu_common.h" + +static struct platform_inst_fw_cap inst_fw_cap_sm8250[] = { + { + .cap_id = PIPE, + .min = PIPE_1, + .max = PIPE_4, + .step_or_mask = 1, + .value = PIPE_4, + .hfi_id = HFI_PROPERTY_PARAM_WORK_ROUTE, + .set = iris_set_pipe, + }, + { + .cap_id = STAGE, + .min = STAGE_1, + .max = STAGE_2, + .step_or_mask = 1, + .value = STAGE_2, + .hfi_id = HFI_PROPERTY_PARAM_WORK_MODE, + .set = iris_set_stage, + }, + { + .cap_id = DEBLOCK, + .min = 0, + .max = 1, + .step_or_mask = 1, + .value = 0, + .hfi_id = HFI_PROPERTY_CONFIG_VDEC_POST_LOOP_DEBLOCKER, + .set = iris_set_u32, + }, +}; + +static struct platform_inst_caps platform_inst_cap_sm8250 = { + .min_frame_width = 128, + .max_frame_width = 8192, + .min_frame_height = 128, + .max_frame_height = 8192, + .max_mbpf = 138240, + .mb_cycles_vsp = 25, + .mb_cycles_vpp = 200, +}; + +static void iris_set_sm8250_preset_registers(struct iris_core *core) +{ + writel(0x0, core->reg_base + 0xB0088); +} + +static const struct icc_info sm8250_icc_table[] = { + { "cpu-cfg", 1000, 1000 }, + { "video-mem", 1000, 15000000 }, +}; + +static const char * const sm8250_clk_reset_table[] = { "bus", "core" }; + +static const struct bw_info sm8250_bw_table_dec[] = { + { ((4096 * 2160) / 256) * 60, 2403000 }, + { ((4096 * 2160) / 256) * 30, 1224000 }, + { ((1920 * 1080) / 256) * 60, 812000 }, + { ((1920 * 1080) / 256) * 30, 416000 }, +}; + +static const char * const sm8250_pmdomain_table[] = { "venus", "vcodec0" }; + +static const char * const sm8250_opp_pd_table[] = { "mx" }; + +static const struct platform_clk_data sm8250_clk_table[] = { + {IRIS_AXI_CLK, "iface" }, + {IRIS_CTRL_CLK, "core" }, + {IRIS_HW_CLK, "vcodec0_core" }, +}; + +static struct tz_cp_config tz_cp_config_sm8250 = { + .cp_start = 0, + .cp_size = 0x25800000, + .cp_nonpixel_start = 0x01000000, + .cp_nonpixel_size = 0x24800000, +}; + +static const u32 sm8250_vdec_input_config_param[] = { + HFI_PROPERTY_PARAM_FRAME_SIZE, + HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE, + HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SELECT, + HFI_PROPERTY_PARAM_UNCOMPRESSED_PLANE_ACTUAL_CONSTRAINTS_INFO, + HFI_PROPERTY_PARAM_BUFFER_COUNT_ACTUAL, + HFI_PROPERTY_PARAM_VDEC_MULTI_STREAM, + HFI_PROPERTY_PARAM_BUFFER_SIZE_ACTUAL, + HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE, +}; + +static const u32 sm8250_dec_ip_int_buf_tbl[] = { + BUF_BIN, + BUF_SCRATCH_1, +}; + +static const u32 sm8250_dec_op_int_buf_tbl[] = { + BUF_DPB, +}; + +struct iris_platform_data sm8250_data = { + .get_instance = iris_hfi_gen1_get_instance, + .init_hfi_command_ops = &iris_hfi_gen1_command_ops_init, + .init_hfi_response_ops = iris_hfi_gen1_response_ops_init, + .vpu_ops = &iris_vpu2_ops, + .set_preset_registers = iris_set_sm8250_preset_registers, + .icc_tbl = sm8250_icc_table, + .icc_tbl_size = ARRAY_SIZE(sm8250_icc_table), + .clk_rst_tbl = sm8250_clk_reset_table, + .clk_rst_tbl_size = ARRAY_SIZE(sm8250_clk_reset_table), + .bw_tbl_dec = sm8250_bw_table_dec, + .bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec), + .pmdomain_tbl = sm8250_pmdomain_table, + .pmdomain_tbl_size = ARRAY_SIZE(sm8250_pmdomain_table), + .opp_pd_tbl = sm8250_opp_pd_table, + .opp_pd_tbl_size = ARRAY_SIZE(sm8250_opp_pd_table), + .clk_tbl = sm8250_clk_table, + .clk_tbl_size = ARRAY_SIZE(sm8250_clk_table), + .dma_mask = GENMASK(31, 29) - 1, + .fwname = "qcom/vpu-1.0/venus.mbn", + .pas_id = IRIS_PAS_ID, + .inst_caps = &platform_inst_cap_sm8250, + .inst_fw_caps = inst_fw_cap_sm8250, + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8250), + .tz_cp_config_data = &tz_cp_config_sm8250, + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, + .num_vpp_pipe = 4, + .max_session_count = 16, + .max_core_mbpf = (8192 * 4352) / 256, + .input_config_params = + sm8250_vdec_input_config_param, + .input_config_params_size = + ARRAY_SIZE(sm8250_vdec_input_config_param), + + .dec_ip_int_buf_tbl = sm8250_dec_ip_int_buf_tbl, + .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8250_dec_ip_int_buf_tbl), + .dec_op_int_buf_tbl = sm8250_dec_op_int_buf_tbl, + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8250_dec_op_int_buf_tbl), +}; diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c index 954cc7c0cc97..f0b83903ad00 100644 --- a/drivers/media/platform/qcom/iris/iris_probe.c +++ b/drivers/media/platform/qcom/iris/iris_probe.c @@ -189,6 +189,34 @@ static void iris_sys_error_handler(struct work_struct *work) iris_core_init(core); } +static bool prefer_venus = true; +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); +module_param(prefer_venus, bool, 0444); + +/* list all platforms supported by only iris driver */ +static const char *const iris_only_platforms[] = { + "qcom,sm8550-iris", + NULL, +}; + +/* list all platforms supported by both venus and iris drivers */ +static const char *const venus_to_iris_migration[] = { + "qcom,sm8250-venus", + NULL, +}; + +static bool video_drv_should_bind(struct device *dev, bool is_iris_driver) +{ + if (of_device_compatible_match(dev->of_node, iris_only_platforms)) + return is_iris_driver; + + /* If it is not in the migration list, use venus */ + if (!of_device_compatible_match(dev->of_node, venus_to_iris_migration)) + return !is_iris_driver; + + return prefer_venus ? !is_iris_driver : is_iris_driver; +} + static int iris_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) u64 dma_mask; int ret; + if (!video_drv_should_bind(&pdev->dev, true)) + return -ENODEV; + core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); if (!core) return -ENOMEM; @@ -324,6 +355,10 @@ static const struct of_device_id iris_dt_match[] = { .compatible = "qcom,sm8550-iris", .data = &sm8550_data, }, + { + .compatible = "qcom,sm8250-venus", + .data = &sm8250_data, + }, { }, }; MODULE_DEVICE_TABLE(of, iris_dt_match);