Re: [PATCH v5 0/3] media: venus: Provide support for selecting encoder/decoder from in-driver

[Vikash Garodia]

Hi Bryan,

On 12/9/2024 5:22 PM, Bryan O'Donoghue wrote:
> v5:
> - Fixes venus_remove_dynamic_nodes() on probe err path - Dikshita
> - Link to v4: https://lore.kernel.org/r/20241128-media-staging-24-11-25-rb3-hw-compat-string-v4-0-fd062b399374@xxxxxxxxxx
> 
> v4:
> 
> - Adds some unavoidable conditional CONFIG_OF_DYNAMIC to fix media-ci testcase # Test build:OF x86_64
> - Added logic for of_changeset_revert() and of_changeset_destroy() on
>   error/remove paths - Bryan
> - Link to v3: https://lore.kernel.org/r/20241127-media-staging-24-11-25-rb3-hw-compat-string-v3-0-ef6bd25e98db@xxxxxxxxxx
> 
> v3:
> - Adds select OF_DYNAMIC to venus/Kconfig to ensure of_changeset_*() is
>   available. Instead of ifdefing and have the fix not work without
>   OF_DYNAMIC, select OF_DYANMIC with venus - linux-media-ci
> - Link to v2: https://lore.kernel.org/r/20241127-media-staging-24-11-25-rb3-hw-compat-string-v2-0-c010fd45f7ff@xxxxxxxxxx
> 
> v2:
> - Removes useless dev_info() leftover from debugging - Bryan
>   Link: https://lore.kernel.org/r/ce9ac473-2f73-4c7a-97b1-08be39f3adb4@xxxxxxxxxx
> - Trivial newline change @ np = of_changeset_create_node(ocs, dev->of_node, node_name); - Bryan
> - Fixes a missing goto identified by smatch - Smatch/Bryan
> - Adds Krzysztof's RB to deprecated - Krzysztof
> - Link to v1: https://lore.kernel.org/r/20241127-media-staging-24-11-25-rb3-hw-compat-string-v1-0-99c16f266b46@xxxxxxxxxx
> 
> v1:
> Various venus patches have been held up due to the misuse of DT to provide
> a configuration input to venus as to which mode a given transcoder should
> be in.
> 
> Link: https://lore.kernel.org/linux-arm-msm/436145fd-d65f-44ec-b950-c434775187ca@xxxxxxxxxx
> Link: https://lore.kernel.org/linux-media/ba40de82-b308-67b1-5751-bb2d95f2b8a5@xxxxxxxxxx/
> 
> This series provides support for static configuration of venus from the resource
> structure via:
> 
> 1. Adding two strings to the resource structure.
>    One string for the decoder one for the encoder.
> 2. The string for each SoC has been matched to the existing in the
>    DT which currently specifies the mode as decoder or encoder.
> 3. New logic in the driver parses the DTB looking for the node name
>    specified for the decoder and encoder .
> 4. If the DTB contains the node name, then no new node is added as
>    we assume to be working with an "old" DTB.
> 5. If the DTB does not contain the specified decoder/encoder string
>    then a new in-memory node is added which contains a compat string
>    consistent with upstream compat strings used to currently select
>    between the decoder and encoder respectively.
> 6. In this way new venus driver entries may be added which respect
>    the requirement to move mode selection out of DTB and into driver.
> 7. Simple instances of decoder/encoder nodes in the yaml schema have been
>    marked as deprecated.
> 8. Since the proposed scheme here always defers to what the DTB says that
>    means it would be possible to remove decoder/encoder entries for the
>    deprecated schema should we choose to do so at a later date but,
>    that step is not taken in this series.
> 9. Some of the upstream encoder/decoder nodes for example sdm630/sdm660
>    also contain clock and power-domain information and have not been
>    updated with the static configuration data or had the schema amended to
>    deprecate values. Because these nodes impart hardware specific
>    information and are already upstream this series proposes to leave
>    those as-is.
> 
> However if this scheme is adopted it should allow for addition of venus for
> both qcs615[1] and sc8280xp[2].
> 
> Other SoCs such as sm8550, sm8650 and beyond are expected to be supported
> by Iris.
> 
> The sm8350 and sm8280xp in the second series would then be able to excise
> the offending compat = "video-encoder" | "video-decoder" in the schema and
> DT.
> 
> I considered making this series an all singing all dancing method to select
> between encoder and decoder for all SoCs but, the objective here is not to
> add functionality but to provide support for configuration in-driver
> consistent with current usage and to do so with a minimal code
> intervention.
> 
> So far I've tested on RB3 by removing:
> 
> video-core0 {
> 	compatible = "venus-decoder";
> };
> 
> video-core1 {
> 	compatible = "venus-encoder";
> };
> 
> This works - the code adds the nodes into memory and the video
> encoder/decoder logic in the plaform code runs.
> 
> Similarly if the nodes are left in-place then no new nodes are added by the
> code in this series and still both encoder and decoder probe.
> 
> Thus proving the code works and will provide support for new platforms
> while also leaving open the option of dropping nodes from upstream.
> 
> I've left the dropping step out for now, it can be implemented later.
> 
> [1] https://lore.kernel.org/linux-arm-msm/20241125-add-venus-for-qcs615-v3-0-5a376b97a68e@xxxxxxxxxxx
> [2] https://lore.kernel.org/linux-media/20230731-topic-8280_venus-v1-0-8c8bbe1983a5@xxxxxxxxxx/
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
Thank you for the changes, looks good to me. Let me come back with some
validation on few SOCs and update here.

Regards,
Vikash
> ---
> Bryan O'Donoghue (3):
>       media: venus: Add support for static video encoder/decoder declarations
>       media: venus: Populate video encoder/decoder nodename entries
>       media: dt-bindings: qcom-venus: Deprecate video-decoder and video-encoder where applicable
> 
>  .../bindings/media/qcom,msm8916-venus.yaml         |  12 +--
>  .../bindings/media/qcom,sc7180-venus.yaml          |  12 +--
>  .../bindings/media/qcom,sc7280-venus.yaml          |  12 +--
>  .../bindings/media/qcom,sdm845-venus-v2.yaml       |  12 +--
>  .../bindings/media/qcom,sm8250-venus.yaml          |  12 +--
>  drivers/media/platform/qcom/venus/Kconfig          |   1 +
>  drivers/media/platform/qcom/venus/core.c           | 104 ++++++++++++++++++++-
>  drivers/media/platform/qcom/venus/core.h           |   4 +
>  8 files changed, 118 insertions(+), 51 deletions(-)
> ---
> base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319
> change-id: 20241127-media-staging-24-11-25-rb3-hw-compat-string-ea3c99938021
> 
> Best regards,