| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f4adbb9-d6ae-4dfd-80e0-4d2680a92f59@collabora.com>
Date: Tue, 18 Jun 2024 12:12:00 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Michael Walle <michael@...le.cc>, chunkuang.hu@...nel.org
Cc: robh@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
p.zabel@...gutronix.de, airlied@...il.com, daniel@...ll.ch,
maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
matthias.bgg@...il.com, shawn.sung@...iatek.com, yu-chang.lee@...iatek.com,
ck.hu@...iatek.com, jitao.shi@...iatek.com, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-mediatek@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
wenst@...omium.org, kernel@...labora.com, sui.jinfeng@...ux.dev,
Alexandre Mergnat <amergnat@...libre.com>
Subject: Re: [PATCH v7 3/3] drm/mediatek: Implement OF graphs support for
display paths
Il 17/06/24 15:24, Michael Walle ha scritto:
> Hi Angelo,
>
>> +/**
>> + * mtk_drm_of_ddp_path_build_one - Build a Display HW Pipeline for a CRTC Path
>> + * @dev: The mediatek-drm device
>> + * @cpath: CRTC Path relative to a VDO or MMSYS
>> + * @out_path: Pointer to an array that will contain the new pipeline
>> + * @out_path_len: Number of entries in the pipeline array
>> + *
>> + * MediaTek SoCs can use different DDP hardware pipelines (or paths) depending
>> + * on the board-specific desired display configuration; this function walks
>> + * through all of the output endpoints starting from a VDO or MMSYS hardware
>> + * instance and builds the right pipeline as specified in device trees.
>> + *
>> + * Return:
>> + * * %0 - Display HW Pipeline successfully built and validated
>> + * * %-ENOENT - Display pipeline was not specified in device tree
>> + * * %-EINVAL - Display pipeline built but validation failed
>> + * * %-ENOMEM - Failure to allocate pipeline array to pass to the caller
>> + */
>> +static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path cpath,
>> + const unsigned int **out_path,
>> + unsigned int *out_path_len)
>> +{
>> + struct device_node *next, *prev, *vdo = dev->parent->of_node;
>> + unsigned int temp_path[DDP_COMPONENT_DRM_ID_MAX] = { 0 };
>> + unsigned int *final_ddp_path;
>> + unsigned short int idx = 0;
>> + bool ovl_adaptor_comp_added = false;
>> + int ret;
>> +
>> + /* Get the first entry for the temp_path array */
>> + ret = mtk_drm_of_get_ddp_ep_cid(vdo, 0, cpath, &next, &temp_path[idx]);
>> + if (ret) {
>> + if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
>> + dev_err(dev, "Adding OVL Adaptor for %pOF\n", next);
>> + ovl_adaptor_comp_added = true;
>> + } else {
>> + if (next)
>> + dev_err(dev, "Invalid component %pOF\n", next);
>> + else
>> + dev_err(dev, "Cannot find first endpoint for path %d\n", cpath);
>> +
>> + return ret;
>> + }
>> + }
>> + idx++;
>> +
>> + /*
>> + * Walk through port outputs until we reach the last valid mediatek-drm component.
>> + * To be valid, this must end with an "invalid" component that is a display node.
>> + */
>> + do {
>> + prev = next;
>> + ret = mtk_drm_of_get_ddp_ep_cid(next, 1, cpath, &next, &temp_path[idx]);
>> + of_node_put(prev);
>> + if (ret) {
>> + of_node_put(next);
>> + break;
>> + }
>> +
>> + /*
>> + * If this is an OVL adaptor exclusive component and one of those
>> + * was already added, don't add another instance of the generic
>> + * DDP_COMPONENT_OVL_ADAPTOR, as this is used only to decide whether
>> + * to probe that component master driver of which only one instance
>> + * is needed and possible.
>> + */
>> + if (temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
>> + if (!ovl_adaptor_comp_added)
>> + ovl_adaptor_comp_added = true;
>> + else
>> + idx--;
>> + }
>> + } while (++idx < DDP_COMPONENT_DRM_ID_MAX);
>> +
>> + /*
>> + * The device component might not be disabled: in that case, don't
>
> Sorry there was a typo in my proposal, This should either be
> "not be enabled" or "be disabled".
>
I even noticed the typo and fixed it, then sent the *not* fixed version. Argh.
>> + * check the last entry and just report that the device is missing.
>> + */
>> + if (ret == -ENODEV)
>> + return ret;
>> +
>
> ..
>
>> +static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node *node,
>> + struct mtk_mmsys_driver_data *data)
>> +{
>> + struct device_node *ep_node;
>> + struct of_endpoint of_ep;
>> + bool output_present[MAX_CRTC] = { false };
>> + bool valid_output_found = false;
>> + int ret;
>> +
>> + for_each_endpoint_of_node(node, ep_node) {
>> + ret = of_graph_parse_endpoint(ep_node, &of_ep);
>> + if (ret) {
>> + dev_err_probe(dev, ret, "Cannot parse endpoint\n");
>> + break;
>> + }
>> +
>> + if (of_ep.id >= MAX_CRTC) {
>> + ret = dev_err_probe(dev, -EINVAL,
>> + "Invalid endpoint%u number\n", of_ep.port);
>> + break;
>> + }
>> +
>> + output_present[of_ep.id] = true;
>> + }
>> +
>> + if (ret) {
>> + of_node_put(ep_node);
>> + return ret;
>> + }
>> +
>> + if (output_present[CRTC_MAIN]) {
>> + ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_MAIN,
>> + &data->main_path, &data->main_len);
>> + if (ret == 0)
>> + valid_output_found = true;
>> + else if (ret != -ENODEV)
>> + return ret;
>> + }
>> +
>> + if (output_present[CRTC_EXT]) {
>> + ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_EXT,
>> + &data->ext_path, &data->ext_len);
>> + if (ret == 0)
>> + valid_output_found = true;
>> + else if (ret != -ENODEV)
>> + return ret;
>> + }
>> +
>> + if (output_present[CRTC_THIRD]) {
>> + ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_THIRD,
>> + &data->third_path, &data->third_len);
>> + if (ret == 0)
>> + valid_output_found = true;
>> + else if (ret != -ENODEV)
>> + return ret;
>> + }
>> +
>> + if (!valid_output_found)
>> + return -ENODEV;
>
> This doesn't work. My proposal just ignored the ENODEV error. Now
> you'll return ENODEV if there is no output for a given mmsys. In my
> case, that is true for the first mmsys. Subsequent mmsys's doesn't
> get probed in that case, it seems.
>
> Anyway, you shouldn't return ENODEV here because disabled just
> means not available, i.e. it should be treated the same as
> "output_present[] == false".
>
Right. Okay, v8 will fix that.
Thanks,
Angelo
Powered by blists - more mailing lists