Re: [PATCH v2 1/2] drm/ttm: Change ttm_device_init to use a struct instead of multiple bools
On Wed, 02 Oct 2024, Thomas Hellström wrote: > The ttm_device_init funcition uses multiple bool arguments. That means > readability in the caller becomes poor, and all callers need to change if > yet another bool is added. > > Instead use a struct with multiple single-bit flags. This addresses both > problems. Prefer it over using defines or enums with explicit bit shifts, > since converting to and from these bit values uses logical operations or > tests which are implicit with the struct usage, and ofc type-checking. > > This is in preparation of adding yet another bool flag parameter to the > function. Funny, the other day Ville and I were throwing ideas around, and we talked about something like this to implement keyword arguments in C. :) Cheers, Jani. -- Jani Nikula, Intel
Re: [PATCH 07/28] drm/i915: Use video aperture helpers
On Mon, 30 Sep 2024, Thomas Zimmermann wrote: > DRM's aperture functions have long been implemented as helpers > under drivers/video/ for use with fbdev. Avoid the DRM wrappers by > calling the video functions directly. > > Signed-off-by: Thomas Zimmermann > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: Tvrtko Ursulin Acked-by: Jani Nikula > --- > drivers/gpu/drm/i915/i915_driver.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > b/drivers/gpu/drm/i915/i915_driver.c > index b3eb35fa5ff8..365329ff8a07 100644 > --- a/drivers/gpu/drm/i915/i915_driver.c > +++ b/drivers/gpu/drm/i915/i915_driver.c > @@ -27,6 +27,7 @@ > * > */ > > +#include > #include > #include > #include > @@ -39,7 +40,6 @@ > #include > #include > > -#include > #include > #include > #include > @@ -485,7 +485,7 @@ static int i915_driver_hw_probe(struct drm_i915_private > *dev_priv) > if (ret) > goto err_perf; > > - ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, > dev_priv->drm.driver); > + ret = aperture_remove_conflicting_pci_devices(pdev, > dev_priv->drm.driver->name); > if (ret) > goto err_ggtt; -- Jani Nikula, Intel
Re: [PATCH v4 0/6] DRM_SET_CLIENT_NAME ioctl
On Fri, 27 Sep 2024, Christian König wrote: > Am 27.09.24 um 10:48 schrieb Pierre-Eric Pelloux-Prayer: >> v4 changelog: >> * DRM_SET_NAME -> DRM_SET_CLIENT_NAME (Dmitry) >> * reject names that would mess up with formatting (Sima), >>and use a stricter filter (isgraph allowed extended ASCII >>which weren't looking great) >> * documentation edits, minor fixups (Dmitry, Trvtko) >> * clarified commit message of commit 3/6 (Trvtko) >> * reworked amdgpu_vm_set_task_info a bit in 4/6 (Trvtko) > > If nobody has any more additional comments on this I'm going to pick it > up and merge it through drm-misc-next by the end of today. AFAICT the userspace is not reviewed and ready for merging [1]. BR, Jani. [1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428 -- Jani Nikula, Intel
Re: [PATCH] drm/display/dsc: Refactor MST DSC Determination Policy
!(fec_cap & DP_FEC_CAPABLE)) {
> + drm_err(mgr->dev, "MST_DSC Failed to retrieve
> fec caps at port %p\n", fec_port);
> + goto out_dsc_fail;
> + }
> + fec_port->fec_capable = true;
> + }
>
> - if (dpcd_ext[DP_DPCD_REV] >= DP_DPCD_REV_14 &&
> - ((dpcd_ext[DP_DOWNSTREAMPORT_PRESENT] &
> DP_DWN_STRM_PORT_PRESENT) &&
> - ((dpcd_ext[DP_DOWNSTREAMPORT_PRESENT] &
> DP_DWN_STRM_PORT_TYPE_MASK)
> - != DP_DWN_STRM_PORT_TYPE_ANALOG)))
> - return immediate_upstream_aux;
> + fec_port = fec_port->parent->port_parent;
> }
>
> - /*
> - * The check below verifies if the MST sink
> - * connected to the GPU is capable of DSC -
> - * therefore the endpoint needs to be
> - * both DSC and FEC capable.
> - */
> - if (drm_dp_dpcd_read(&port->aux,
> -DP_DSC_SUPPORT, &endpoint_dsc, 1) != 1)
> - return NULL;
> - if (drm_dp_dpcd_read(&port->aux,
> -DP_FEC_CAPABILITY, &endpoint_fec, 1) != 1)
> - return NULL;
> - if ((endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED) &&
> -(endpoint_fec & DP_FEC_CAPABLE))
> - return &port->aux;
> + /* Ensure fec between source and the connected DPRx */
> + if ((drm_dp_dpcd_read(mgr->aux, DP_FEC_CAPABILITY, &fec_cap, 1) != 1) ||
> + !(fec_cap & DP_FEC_CAPABLE)) {
> + drm_err(mgr->dev, "MST_DSC fec not supported between source and
> the connected DPRx\n");
> + goto out_dsc_fail;
> + }
>
> - return NULL;
> + return;
> +
> +out_dsc_fail:
> + port->dsc_aux = NULL;
> + port->dsc_passthrough_aux = NULL;
> + return;
> }
> EXPORT_SYMBOL(drm_dp_mst_dsc_aux_for_port);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index a1fcedfd404b..d39a7c6f5bfa 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3219,7 +3219,7 @@ intel_dp_sink_set_dsc_passthrough(const struct
> intel_connector *connector,
> {
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct drm_dp_aux *aux = connector->port ?
> - connector->port->passthrough_aux : NULL;
> + connector->port->dsc_passthrough_aux : NULL;
>
> if (!aux)
> return;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 15541932b809..73cb1c673525 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1699,7 +1699,8 @@ static struct drm_connector
> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>
> intel_dp_init_modeset_retry_work(intel_connector);
>
> - intel_connector->dp.dsc_decompression_aux =
> drm_dp_mst_dsc_aux_for_port(port);
> + drm_dp_mst_dsc_aux_for_port(port);
> + intel_connector->dp.dsc_decompression_aux = port->dsc_aux;
> intel_dp_mst_read_decompression_port_dsc_caps(intel_dp,
> intel_connector);
> intel_connector->dp.dsc_hblank_expansion_quirk =
> detect_dsc_hblank_expansion_quirk(intel_connector);
> diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
> index a6f8b098c56f..fa454506866b 100644
> --- a/include/drm/display/drm_dp.h
> +++ b/include/drm/display/drm_dp.h
> @@ -980,6 +980,7 @@
> #define DP_BRANCH_REVISION_START0x509
> #define DP_BRANCH_HW_REV0x509
> #define DP_BRANCH_SW_REV0x50A
> +#define DP_BRANCH_VENDOR_SPECIFIC_START 0x50C
>
> /* Link/Sink Device Power Control */
> #define DP_SET_POWER0x600
> diff --git a/include/drm/display/drm_dp_helper.h
> b/include/drm/display/drm_dp_helper.h
> index 279624833ea9..5eb583004d00 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -643,6 +643,7 @@ struct drm_dp_dpcd_ident {
> u8 hw_rev;
> u8 sw_major_rev;
> u8 sw_minor_rev;
> + u8 vendor_data[4];
> } __packed;
>
> /**
> @@ -711,6 +712,13 @@ enum drm_dp_quirk {
>* requires enabling DSC.
>*/
> DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC,
> + /**
> + * @DP_DPCD_QUIRK_DSC_MUST_ROOT_DECOMPRESSION:
> + *
> + * The device has internal virutual dpcd with dsc decoding cap,
> + * but dsc decoding must be done at root mstb.
> + */
> + DP_DPCD_QUIRK_DSC_MUST_ROOT_DECOMPRESSION,
> };
>
> /**
> diff --git a/include/drm/display/drm_dp_mst_helper.h
> b/include/drm/display/drm_dp_mst_helper.h
> index f6a1cbb0f600..b04ca4a97af2 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -135,7 +135,8 @@ struct drm_dp_mst_port {
>*/
> struct drm_dp_mst_branch *mstb;
> struct drm_dp_aux aux; /* i2c bus for this port? */
> - struct drm_dp_aux *passthrough_aux;
> + struct drm_dp_aux *dsc_aux;
> + struct drm_dp_aux *dsc_passthrough_aux;
> struct drm_dp_mst_branch *parent;
>
> struct drm_connector *connector;
> @@ -956,7 +957,7 @@ bool drm_dp_mst_port_is_logical(struct drm_dp_mst_port
> *port)
> }
>
> struct drm_dp_aux *drm_dp_mst_aux_for_parent(struct drm_dp_mst_port *port);
> -struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port);
> +void drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port);
>
> static inline struct drm_dp_mst_topology_state *
> to_drm_dp_mst_topology_state(struct drm_private_state *state)
--
Jani Nikula, Intel
Re: [PATCH] drm/amdgpu: enable -Wformat-truncation
On Tue, 03 Sep 2024, Hamza Mahfooz wrote: > It is enabled by W=1 and amdgpu has a clean build with it enabled. So, > to make sure we block future instances of it from showing up on > our driver, enable it by default for the module. Would prefer enabling it by default across the subsystem [1]. BR, Jani. [1] https://lore.kernel.org/r/cover.1716479340.git.jani.nik...@intel.com > > Signed-off-by: Hamza Mahfooz > --- > drivers/gpu/drm/amd/amdgpu/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile > b/drivers/gpu/drm/amd/amdgpu/Makefile > index 34943b866687..64adadb56fd6 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > @@ -41,6 +41,7 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > > # Locally disable W=1 warnings enabled in drm subsystem Makefile > subdir-ccflags-y += -Wno-override-init > +subdir-ccflags-y += $(call cc-option, -Wformat-truncation) > subdir-ccflags-$(CONFIG_DRM_AMDGPU_WERROR) += -Werror > > amdgpu-y := amdgpu_drv.o -- Jani Nikula, Intel
Re: [RESEND 3/3] drm/amd/display: switch to guid_gen() to generate valid GUIDs
On Wed, 28 Aug 2024, Harry Wentland wrote:
> On 2024-08-28 09:58, Alex Deucher wrote:
>> On Wed, Aug 28, 2024 at 9:53 AM Jani Nikula wrote:
>>>
>>> On Wed, 28 Aug 2024, Daniel Vetter wrote:
>>>> On Mon, Aug 12, 2024 at 03:23:12PM +0300, Jani Nikula wrote:
>>>>> Instead of just smashing jiffies into a GUID, use guid_gen() to generate
>>>>> RFC 4122 compliant GUIDs.
>>>>>
>>>>> Signed-off-by: Jani Nikula
>>>>>
>>>>> ---
>>>>>
>>>>> Side note, it baffles me why amdgpu has a copy of this instead of
>>>>> plumbing it into drm mst code.
>>>>
>>>> Yeah ec5fa9fcdeca ("drm/amd/display: Adjust the MST resume flow") promised
>>>> a follow-up, but that seems to have never materialized. Really should
>>>> materialize though. Patch lgtm
>>>>
>>>> Reviewed-by: Daniel Vetter
>>>
>>> Thanks!
>>>
>>> Cc: AMD folks, ack for merging the series via drm-misc-next?
>>
>> Unless Harry has any objections,
>> Acked-by: Alex Deucher
>>
>
> Acked-by: Harry Wentland
Thanks for the reviews and acks, pushed the series to drm-misc-next.
BR,
Jani.
--
Jani Nikula, Intel
Re: [RESEND 3/3] drm/amd/display: switch to guid_gen() to generate valid GUIDs
On Wed, 28 Aug 2024, Daniel Vetter wrote:
> On Mon, Aug 12, 2024 at 03:23:12PM +0300, Jani Nikula wrote:
>> Instead of just smashing jiffies into a GUID, use guid_gen() to generate
>> RFC 4122 compliant GUIDs.
>>
>> Signed-off-by: Jani Nikula
>>
>> ---
>>
>> Side note, it baffles me why amdgpu has a copy of this instead of
>> plumbing it into drm mst code.
>
> Yeah ec5fa9fcdeca ("drm/amd/display: Adjust the MST resume flow") promised
> a follow-up, but that seems to have never materialized. Really should
> materialize though. Patch lgtm
>
> Reviewed-by: Daniel Vetter
Thanks!
Cc: AMD folks, ack for merging the series via drm-misc-next?
BR,
Jani.
>
>
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++-
>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 72c10fc2c890..ce05e7e2a383 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -2568,9 +2568,9 @@ static int dm_late_init(void *handle)
>>
>> static void resume_mst_branch_status(struct drm_dp_mst_topology_mgr *mgr)
>> {
>> +u8 buf[UUID_SIZE];
>> +guid_t guid;
>> int ret;
>> -u8 guid[16];
>> -u64 tmp64;
>>
>> mutex_lock(&mgr->lock);
>> if (!mgr->mst_primary)
>> @@ -2591,26 +2591,27 @@ static void resume_mst_branch_status(struct
>> drm_dp_mst_topology_mgr *mgr)
>> }
>>
>> /* Some hubs forget their guids after they resume */
>> -ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
>> -if (ret != 16) {
>> +ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, buf, sizeof(buf));
>> +if (ret != sizeof(buf)) {
>> drm_dbg_kms(mgr->dev, "dpcd read failed - undocked during
>> suspend?\n");
>> goto out_fail;
>> }
>>
>> -if (memchr_inv(guid, 0, 16) == NULL) {
>> -tmp64 = get_jiffies_64();
>> -memcpy(&guid[0], &tmp64, sizeof(u64));
>> -memcpy(&guid[8], &tmp64, sizeof(u64));
>> +import_guid(&guid, buf);
>>
>> -ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, guid, 16);
>> +if (guid_is_null(&guid)) {
>> +guid_gen(&guid);
>> +export_guid(buf, &guid);
>>
>> -if (ret != 16) {
>> +ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, buf, sizeof(buf));
>> +
>> +if (ret != sizeof(buf)) {
>> drm_dbg_kms(mgr->dev, "check mstb guid failed -
>> undocked during suspend?\n");
>> goto out_fail;
>> }
>> }
>>
>> -import_guid(&mgr->mst_primary->guid, guid);
>> +guid_copy(&mgr->mst_primary->guid, &guid);
>>
>> out_fail:
>> mutex_unlock(&mgr->lock);
>> --
>> 2.39.2
>>
--
Jani Nikula, Intel
Re: [RESEND 3/3] drm/amd/display: switch to guid_gen() to generate valid GUIDs
On Wed, 28 Aug 2024, Hamza Mahfooz wrote:
> On 8/12/24 08:23, Jani Nikula wrote:
>> Instead of just smashing jiffies into a GUID, use guid_gen() to generate
>> RFC 4122 compliant GUIDs.
>>
>> Signed-off-by: Jani Nikula
>>
>> ---
>
> Acked-by: Hamza Mahfooz
>
> I would prefer to take this series through the amdgpu tree though,
> assuming nobody minds.
How long is it going to take for that to get synced back to
drm-misc-next though?
BR,
Jani.
>
>>
>> Side note, it baffles me why amdgpu has a copy of this instead of
>> plumbing it into drm mst code.
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++-
>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 72c10fc2c890..ce05e7e2a383 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -2568,9 +2568,9 @@ static int dm_late_init(void *handle)
>>
>> static void resume_mst_branch_status(struct drm_dp_mst_topology_mgr *mgr)
>> {
>> +u8 buf[UUID_SIZE];
>> +guid_t guid;
>> int ret;
>> -u8 guid[16];
>> -u64 tmp64;
>>
>> mutex_lock(&mgr->lock);
>> if (!mgr->mst_primary)
>> @@ -2591,26 +2591,27 @@ static void resume_mst_branch_status(struct
>> drm_dp_mst_topology_mgr *mgr)
>> }
>>
>> /* Some hubs forget their guids after they resume */
>> -ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
>> -if (ret != 16) {
>> +ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, buf, sizeof(buf));
>> +if (ret != sizeof(buf)) {
>> drm_dbg_kms(mgr->dev, "dpcd read failed - undocked during
>> suspend?\n");
>> goto out_fail;
>> }
>>
>> -if (memchr_inv(guid, 0, 16) == NULL) {
>> -tmp64 = get_jiffies_64();
>> -memcpy(&guid[0], &tmp64, sizeof(u64));
>> -memcpy(&guid[8], &tmp64, sizeof(u64));
>> +import_guid(&guid, buf);
>>
>> -ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, guid, 16);
>> +if (guid_is_null(&guid)) {
>> +guid_gen(&guid);
>> +export_guid(buf, &guid);
>>
>> -if (ret != 16) {
>> +ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, buf, sizeof(buf));
>> +
>> +if (ret != sizeof(buf)) {
>> drm_dbg_kms(mgr->dev, "check mstb guid failed -
>> undocked during suspend?\n");
>> goto out_fail;
>> }
>> }
>>
>> -import_guid(&mgr->mst_primary->guid, guid);
>> +guid_copy(&mgr->mst_primary->guid, &guid);
>>
>> out_fail:
>> mutex_unlock(&mgr->lock);
--
Jani Nikula, Intel
Re: [RESEND 3/3] drm/amd/display: switch to guid_gen() to generate valid GUIDs
On Wed, 28 Aug 2024, Jani Nikula wrote:
> On Wed, 28 Aug 2024, Hamza Mahfooz wrote:
>> On 8/12/24 08:23, Jani Nikula wrote:
>>> Instead of just smashing jiffies into a GUID, use guid_gen() to generate
>>> RFC 4122 compliant GUIDs.
>>>
>>> Signed-off-by: Jani Nikula
>>>
>>> ---
>>
>> Acked-by: Hamza Mahfooz
>>
>> I would prefer to take this series through the amdgpu tree though,
>> assuming nobody minds.
>
> How long is it going to take for that to get synced back to
> drm-misc-next though?
Also getting acks from Alex and Harry to merge via drm-misc-next.
BR,
Jani.
>
> BR,
> Jani.
>
>
>>
>>>
>>> Side note, it baffles me why amdgpu has a copy of this instead of
>>> plumbing it into drm mst code.
>>> ---
>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++-
>>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 72c10fc2c890..ce05e7e2a383 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -2568,9 +2568,9 @@ static int dm_late_init(void *handle)
>>>
>>> static void resume_mst_branch_status(struct drm_dp_mst_topology_mgr *mgr)
>>> {
>>> + u8 buf[UUID_SIZE];
>>> + guid_t guid;
>>> int ret;
>>> - u8 guid[16];
>>> - u64 tmp64;
>>>
>>> mutex_lock(&mgr->lock);
>>> if (!mgr->mst_primary)
>>> @@ -2591,26 +2591,27 @@ static void resume_mst_branch_status(struct
>>> drm_dp_mst_topology_mgr *mgr)
>>> }
>>>
>>> /* Some hubs forget their guids after they resume */
>>> - ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
>>> - if (ret != 16) {
>>> + ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, buf, sizeof(buf));
>>> + if (ret != sizeof(buf)) {
>>> drm_dbg_kms(mgr->dev, "dpcd read failed - undocked during
>>> suspend?\n");
>>> goto out_fail;
>>> }
>>>
>>> - if (memchr_inv(guid, 0, 16) == NULL) {
>>> - tmp64 = get_jiffies_64();
>>> - memcpy(&guid[0], &tmp64, sizeof(u64));
>>> - memcpy(&guid[8], &tmp64, sizeof(u64));
>>> + import_guid(&guid, buf);
>>>
>>> - ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, guid, 16);
>>> + if (guid_is_null(&guid)) {
>>> + guid_gen(&guid);
>>> + export_guid(buf, &guid);
>>>
>>> - if (ret != 16) {
>>> + ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, buf, sizeof(buf));
>>> +
>>> + if (ret != sizeof(buf)) {
>>> drm_dbg_kms(mgr->dev, "check mstb guid failed -
>>> undocked during suspend?\n");
>>> goto out_fail;
>>> }
>>> }
>>>
>>> - import_guid(&mgr->mst_primary->guid, guid);
>>> + guid_copy(&mgr->mst_primary->guid, &guid);
>>>
>>> out_fail:
>>> mutex_unlock(&mgr->lock);
--
Jani Nikula, Intel
Re: [PATCH] drm/radeon: Switch radeon_connector to struct drm_edid
+871,7 @@ radeon_lvds_detect(struct drm_connector *connector, bool
> force)
>
> /* check for edid as well */
> radeon_connector_get_edid(connector);
> - if (radeon_connector->edid)
> + if (radeon_connector->drm_edid)
> ret = connector_status_connected;
> /* check acpi lid status ??? */
>
> @@ -1012,13 +1014,12 @@ radeon_vga_detect(struct drm_connector *connector,
> bool force)
> radeon_connector_free_edid(connector);
> radeon_connector_get_edid(connector);
>
> - if (!radeon_connector->edid) {
> + if (!radeon_connector->drm_edid) {
> DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
> connector->name);
> ret = connector_status_connected;
> } else {
> - radeon_connector->use_digital =
> - !!(radeon_connector->edid->input &
> DRM_EDID_INPUT_DIGITAL);
> + radeon_connector->use_digital =
> drm_edid_is_digital(radeon_connector->drm_edid);
>
> /* some oems have boards with separate digital and
> analog connectors
>* with a shared ddc line (often vga + hdmi)
> @@ -1270,7 +1271,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool
> force)
> radeon_connector_free_edid(connector);
> radeon_connector_get_edid(connector);
>
> - if (!radeon_connector->edid) {
> + if (!radeon_connector->drm_edid) {
> DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
> connector->name);
> /* rs690 seems to have a problem with connectors not
> existing and always
> @@ -1286,8 +1287,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool
> force)
> broken_edid = true; /* defer use_digital to
> later */
> }
> } else {
> - radeon_connector->use_digital =
> - !!(radeon_connector->edid->input &
> DRM_EDID_INPUT_DIGITAL);
> + radeon_connector->use_digital =
> drm_edid_is_digital(radeon_connector->drm_edid);
>
> /* some oems have boards with separate digital and
> analog connectors
>* with a shared ddc line (often vga + hdmi)
> diff --git a/drivers/gpu/drm/radeon/radeon_mode.h
> b/drivers/gpu/drm/radeon/radeon_mode.h
> index 421c83fc70dc..ae1d91cd93ec 100644
> --- a/drivers/gpu/drm/radeon/radeon_mode.h
> +++ b/drivers/gpu/drm/radeon/radeon_mode.h
> @@ -38,7 +38,6 @@
> #include
> #include
>
> -struct edid;
> struct drm_edid;
> struct radeon_bo;
> struct radeon_device;
> @@ -521,7 +520,7 @@ struct radeon_connector {
> bool use_digital;
> /* we need to mind the EDID between detect
> and get modes due to analog/digital/tvencoder */
> - struct edid *edid;
> + const struct drm_edid *drm_edid;
> void *con_priv;
> bool dac_load_detect;
> bool detected_by_load; /* if the connection status was determined by
> load */
> @@ -843,7 +842,7 @@ radeon_get_crtc_scanout_position(struct drm_crtc *crtc,
> bool in_vblank_irq,
>const struct drm_display_mode *mode);
>
> extern bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev);
> -extern struct edid *
> +extern const struct drm_edid *
> radeon_bios_get_hardcoded_edid(struct radeon_device *rdev);
> extern bool radeon_atom_get_clock_info(struct drm_device *dev);
> extern bool radeon_combios_get_clock_info(struct drm_device *dev);
>
> ---
> base-commit: 19cff16559a4f2d763faf4f8392bf86d3a21b93c
> change-id: 20240818-radeon-drm_edid-9f0cec36e227
>
> Best regards,
--
Jani Nikula, Intel
Re: [PATCH 10/12] drm/edid: add a helper to compare two EDIDs
On Sun, 18 Aug 2024, Thomas Weißschuh wrote:
> As struct drm_edid is opaque, drivers can't directly memcmp() the
> contained data. Add a helper to provide this functionality.
I'm not sure why drivers would need to compare EDIDs.
The only user was added in commit eb815442e840 ("drm/amd/display: don't
create new dc_sink if nothing changed at detection") with absolutely no
explanation why.
Other drivers use connector->epoch_counter to see if the EDID or status
changed.
BR,
Jani.
>
> Signed-off-by: Thomas Weißschuh
> ---
> drivers/gpu/drm/drm_edid.c | 18 ++
> include/drm/drm_edid.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 69fb11741abd..c2493c983a64 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1840,6 +1840,24 @@ static bool drm_edid_eq(const struct drm_edid
> *drm_edid,
> return true;
> }
>
> +/**
> + * drm_edid_equal - compare two EDID
> + * @drm_edid_a: First EDID data
> + * @drm_edid_b: Second EDID data
> + *
> + * Compare two EDIDs for equality (including extensions)
> + *
> + * Return: True if the EDIDs are equal, false otherwise.
> + */
> +bool drm_edid_equal(const struct drm_edid *drm_edid_a, const struct drm_edid
> *drm_edid_b)
> +{
> + if (!drm_edid_b)
> + return !drm_edid_a;
> +
> + return drm_edid_eq(drm_edid_a, drm_edid_b->edid, drm_edid_b->size);
> +}
> +EXPORT_SYMBOL(drm_edid_equal);
> +
> enum edid_block_status {
> EDID_BLOCK_OK = 0,
> EDID_BLOCK_READ_FAIL,
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index a5b377c4a342..35b40a9d3350 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -456,6 +456,7 @@ drm_display_mode_from_cea_vic(struct drm_device *dev,
> const struct drm_edid *drm_edid_alloc(const void *edid, size_t size);
> const struct drm_edid *drm_edid_dup(const struct drm_edid *drm_edid);
> void drm_edid_free(const struct drm_edid *drm_edid);
> +bool drm_edid_equal(const struct drm_edid *drm_edid_a, const struct drm_edid
> *drm_edid_b);
> bool drm_edid_valid(const struct drm_edid *drm_edid);
> const struct edid *drm_edid_raw(const struct drm_edid *drm_edid);
> const struct drm_edid *drm_edid_read(struct drm_connector *connector);
--
Jani Nikula, Intel
Re: [PATCH 09/12] drm/amd/display: Switch amdgpu_dm_connector to struct drm_edid
m_connector_cleanup(connector);
> drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> @@ -182,7 +182,7 @@ amdgpu_dm_mst_connector_early_unregister(struct
> drm_connector *connector)
>
> dc_sink_release(dc_sink);
> aconnector->dc_sink = NULL;
> - aconnector->edid = NULL;
> + aconnector->drm_edid = NULL;
> aconnector->dsc_aux = NULL;
> port->passthrough_aux = NULL;
> }
> @@ -302,12 +302,13 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> if (!aconnector)
> return drm_add_edid_modes(connector, NULL);
>
> - if (!aconnector->edid) {
> - struct edid *edid;
> + if (!aconnector->drm_edid) {
> + const struct drm_edid *drm_edid;
>
> - edid = drm_dp_mst_get_edid(connector,
> &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> + drm_edid = drm_dp_mst_edid_read(connector,
> &aconnector->mst_root->mst_mgr,
> + aconnector->mst_output_port);
>
> - if (!edid) {
> + if (!drm_edid) {
> amdgpu_dm_set_mst_status(&aconnector->mst_status,
> MST_REMOTE_EDID, false);
>
> @@ -344,7 +345,7 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> return ret;
> }
>
> - aconnector->edid = edid;
> + aconnector->drm_edid = drm_edid;
> amdgpu_dm_set_mst_status(&aconnector->mst_status,
> MST_REMOTE_EDID, true);
> }
> @@ -361,7 +362,7 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
> dc_sink = dc_link_add_remote_sink(
> aconnector->dc_link,
> - aconnector->edid,
> + drm_edid_raw(aconnector->drm_edid),
> &init_params);
>
> if (!dc_sink) {
> @@ -403,7 +404,7 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
>
> if (aconnector->dc_sink) {
> amdgpu_dm_update_freesync_caps(
> - connector, aconnector->edid);
> + connector, aconnector->drm_edid);
>
> #if defined(CONFIG_DRM_AMD_DC_FP)
> if (!validate_dsc_caps_on_connector(aconnector))
> @@ -417,10 +418,9 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> }
> }
>
> - drm_connector_update_edid_property(
> - &aconnector->base, aconnector->edid);
> + drm_edid_connector_update(&aconnector->base, aconnector->drm_edid);
>
> - ret = drm_add_edid_modes(connector, aconnector->edid);
> + ret = drm_edid_connector_add_modes(connector);
>
> return ret;
> }
> @@ -498,7 +498,7 @@ dm_dp_mst_detect(struct drm_connector *connector,
>
> dc_sink_release(aconnector->dc_sink);
> aconnector->dc_sink = NULL;
> - aconnector->edid = NULL;
> + aconnector->drm_edid = NULL;
> aconnector->dsc_aux = NULL;
> port->passthrough_aux = NULL;
--
Jani Nikula, Intel
Re: [PATCH 03/12] drm/edid: constify argument of drm_edid_is_valid()
On Sun, 18 Aug 2024, Thomas Weißschuh wrote:
> drm_edid_is_valid() does not modify its argument, so mark it as const.
That's not true.
BR,
Jani.
>
> Signed-off-by: Thomas Weißschuh
> ---
> drivers/gpu/drm/drm_edid.c | 2 +-
> include/drm/drm_edid.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index f68a41eeb1fa..69fb11741abd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2034,7 +2034,7 @@ EXPORT_SYMBOL(drm_edid_block_valid);
> *
> * Return: True if the EDID data is valid, false otherwise.
> */
> -bool drm_edid_is_valid(struct edid *edid)
> +bool drm_edid_is_valid(const struct edid *edid)
> {
> int i;
>
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 6bdfa254a1c1..a5b377c4a342 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -442,7 +442,7 @@ int drm_add_modes_noedid(struct drm_connector *connector,
> int drm_edid_header_is_valid(const void *edid);
> bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
> bool *edid_corrupt);
> -bool drm_edid_is_valid(struct edid *edid);
> +bool drm_edid_is_valid(const struct edid *edid);
> void drm_edid_get_monitor_name(const struct edid *edid, char *name,
> int buflen);
> struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
--
Jani Nikula, Intel
Re: [PATCH 83/86] drm/{i915,xe}: Run DRM default client setup
On Fri, 16 Aug 2024, Thomas Zimmermann wrote:
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 206328387150..7a28396abb25 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -17,6 +17,8 @@
> #include
> #include
>
> +#include "intel_fbdev.h"
> +
> #include "display/xe_display.h"
> #include "instructions/xe_gpu_commands.h"
> #include "regs/xe_gt_regs.h"
> @@ -267,6 +269,7 @@ static struct drm_driver driver = {
>
> .dumb_create = xe_bo_dumb_create,
> .dumb_map_offset = drm_gem_ttm_dumb_map_offset,
> + INTEL_FBDEV_DRIVER_OPS,
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = xe_drm_client_fdinfo,
> #endif
Basically xe_device.c should have close to zero idea about display
details, and should not include intel_fbdev.h directly.
There's a xe_display_driver_set_hooks() call that is the right place to
set the driver->fbdev_probe hook.
It's a bit of a bummer in the sense that this prevents struct drm_driver
from being const, but that's how it already is for xe.
BR,
Jani.
--
Jani Nikula, Intel
Re: [PATCH v4 1/3] drm: Add panel backlight quirks
T_SYMBOL(drm_get_panel_min_brightness_quirk);
> +
> +MODULE_DESCRIPTION("Quirks for panel backlight overrides");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
> index 70775748d243..267711028dd4 100644
> --- a/include/drm/drm_utils.h
> +++ b/include/drm/drm_utils.h
> @@ -11,9 +11,12 @@
> #define __DRM_UTILS_H__
>
> #include
> +#include
Please prefer forward declarations over includes where possible.
Here,
struct drm_edid;
is sufficient.
BR,
Jani.
>
> int drm_get_panel_orientation_quirk(int width, int height);
>
> +int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid);
> +
> signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
>
> #endif
--
Jani Nikula, Intel
[RESEND 1/3] drm/mst: switch to guid_t type for GUID
The kernel has a guid_t type for GUIDs. Switch to using it, but avoid
any functional changes here.
Signed-off-by: Jani Nikula
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
drivers/gpu/drm/display/drm_dp_mst_topology.c | 67 +++
include/drm/display/drm_dp_mst_helper.h | 12 ++--
3 files changed, 45 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 7e7929f24ae4..72c10fc2c890 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2610,7 +2610,7 @@ static void resume_mst_branch_status(struct
drm_dp_mst_topology_mgr *mgr)
}
}
- memcpy(mgr->mst_primary->guid, guid, 16);
+ import_guid(&mgr->mst_primary->guid, guid);
out_fail:
mutex_unlock(&mgr->lock);
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 379a449a28a2..39f1dc45004e 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -89,7 +89,7 @@ static int drm_dp_send_enum_path_resources(struct
drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_branch *mstb,
struct drm_dp_mst_port *port);
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
-u8 *guid);
+guid_t *guid);
static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port);
static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port);
@@ -801,7 +801,7 @@ static bool drm_dp_sideband_parse_link_address(const struct
drm_dp_mst_topology_
int idx = 1;
int i;
- memcpy(repmsg->u.link_addr.guid, &raw->msg[idx], 16);
+ import_guid(&repmsg->u.link_addr.guid, &raw->msg[idx]);
idx += 16;
repmsg->u.link_addr.nports = raw->msg[idx] & 0xf;
idx++;
@@ -829,7 +829,7 @@ static bool drm_dp_sideband_parse_link_address(const struct
drm_dp_mst_topology_
idx++;
if (idx > raw->curlen)
goto fail_len;
- memcpy(repmsg->u.link_addr.ports[i].peer_guid,
&raw->msg[idx], 16);
+ import_guid(&repmsg->u.link_addr.ports[i].peer_guid,
&raw->msg[idx]);
idx += 16;
if (idx > raw->curlen)
goto fail_len;
@@ -1029,7 +1029,7 @@ static bool drm_dp_sideband_parse_reply(const struct
drm_dp_mst_topology_mgr *mg
msg->req_type = (raw->msg[0] & 0x7f);
if (msg->reply_type == DP_SIDEBAND_REPLY_NAK) {
- memcpy(msg->u.nak.guid, &raw->msg[1], 16);
+ import_guid(&msg->u.nak.guid, &raw->msg[1]);
msg->u.nak.reason = raw->msg[17];
msg->u.nak.nak_data = raw->msg[18];
return false;
@@ -1078,7 +1078,7 @@ drm_dp_sideband_parse_connection_status_notify(const
struct drm_dp_mst_topology_
if (idx > raw->curlen)
goto fail_len;
- memcpy(msg->u.conn_stat.guid, &raw->msg[idx], 16);
+ import_guid(&msg->u.conn_stat.guid, &raw->msg[idx]);
idx += 16;
if (idx > raw->curlen)
goto fail_len;
@@ -1107,7 +1107,7 @@ static bool
drm_dp_sideband_parse_resource_status_notify(const struct drm_dp_mst
if (idx > raw->curlen)
goto fail_len;
- memcpy(msg->u.resource_stat.guid, &raw->msg[idx], 16);
+ import_guid(&msg->u.resource_stat.guid, &raw->msg[idx]);
idx += 16;
if (idx > raw->curlen)
goto fail_len;
@@ -2174,20 +2174,24 @@ ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
offset, size, buffer);
}
-static int drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid)
+static int drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, guid_t *guid)
{
int ret = 0;
- memcpy(mstb->guid, guid, 16);
+ guid_copy(&mstb->guid, guid);
+
+ if (!drm_dp_validate_guid(mstb->mgr, &mstb->guid)) {
+ u8 buf[UUID_SIZE];
+
+ export_guid(buf, &mstb->guid);
- if (!drm_dp_validate_guid(mstb->mgr, mstb->guid)) {
if (mstb->port_parent) {
ret = drm_dp_send_dpcd_write(mstb->mgr,
mstb->port_parent,
-DP_GUID, 16
[RESEND 2/3] drm/mst: switch to guid_gen() to generate valid GUIDs
Instead of just smashing jiffies into a GUID, use guid_gen() to generate
RFC 4122 compliant GUIDs.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 39f1dc45004e..38a9a1441e62 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2700,18 +2700,10 @@ static void drm_dp_mst_queue_probe_work(struct
drm_dp_mst_topology_mgr *mgr)
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
guid_t *guid)
{
- u64 salt;
- u8 buf[UUID_SIZE];
-
if (!guid_is_null(guid))
return true;
- salt = get_jiffies_64();
-
- memcpy(&buf[0], &salt, sizeof(u64));
- memcpy(&buf[8], &salt, sizeof(u64));
-
- import_guid(guid, buf);
+ guid_gen(guid);
return false;
}
--
2.39.2
[RESEND 3/3] drm/amd/display: switch to guid_gen() to generate valid GUIDs
Instead of just smashing jiffies into a GUID, use guid_gen() to generate
RFC 4122 compliant GUIDs.
Signed-off-by: Jani Nikula
---
Side note, it baffles me why amdgpu has a copy of this instead of
plumbing it into drm mst code.
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++-
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 72c10fc2c890..ce05e7e2a383 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2568,9 +2568,9 @@ static int dm_late_init(void *handle)
static void resume_mst_branch_status(struct drm_dp_mst_topology_mgr *mgr)
{
+ u8 buf[UUID_SIZE];
+ guid_t guid;
int ret;
- u8 guid[16];
- u64 tmp64;
mutex_lock(&mgr->lock);
if (!mgr->mst_primary)
@@ -2591,26 +2591,27 @@ static void resume_mst_branch_status(struct
drm_dp_mst_topology_mgr *mgr)
}
/* Some hubs forget their guids after they resume */
- ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
- if (ret != 16) {
+ ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, buf, sizeof(buf));
+ if (ret != sizeof(buf)) {
drm_dbg_kms(mgr->dev, "dpcd read failed - undocked during
suspend?\n");
goto out_fail;
}
- if (memchr_inv(guid, 0, 16) == NULL) {
- tmp64 = get_jiffies_64();
- memcpy(&guid[0], &tmp64, sizeof(u64));
- memcpy(&guid[8], &tmp64, sizeof(u64));
+ import_guid(&guid, buf);
- ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, guid, 16);
+ if (guid_is_null(&guid)) {
+ guid_gen(&guid);
+ export_guid(buf, &guid);
- if (ret != 16) {
+ ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, buf, sizeof(buf));
+
+ if (ret != sizeof(buf)) {
drm_dbg_kms(mgr->dev, "check mstb guid failed -
undocked during suspend?\n");
goto out_fail;
}
}
- import_guid(&mgr->mst_primary->guid, guid);
+ guid_copy(&mgr->mst_primary->guid, &guid);
out_fail:
mutex_unlock(&mgr->lock);
--
2.39.2
Re: [PATCH v5 0/9] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
On Wed, 07 Aug 2024, Melissa Wen wrote: > Here AMD display driver migrates from open struct edid to opaque > drm_edid. This version works on top of amd/drm-next branch since > amd-staging-drm-next doesn't have the commits that support > drm_edid_product_id[1]. It's mostly addressing Alex Hung's feedback > from the previous version. FWIW, I glanced through this and didn't spot anything out of the ordinary. That said, I also didn't do detailed review of it, so I'm not providing my R-b. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors
On Thu, 01 Aug 2024, Thomas Zimmermann wrote: > Hi > > Am 01.08.24 um 14:33 schrieb Jani Nikula: >> On Mon, 01 Jul 2024, Xaver Hugl wrote: >>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl >>> : >>>> Merging can only happen once a real world userspace application has >>>> implemented support for it. I'll try to do that sometime next week in >>>> KWin >>> Here's the promised implementation: >>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028 >> The requirement is that the userspace patches must be reviewed and ready >> for merging into a suitable and canonical upstream project. >> >> Are they? > > I already saw this series in today's PR for drm-misc-next. :/ Exactly what triggered the question! BR, Jani. > > Best regards > Thomas > >> >> >> BR, >> Jani. >> >> >>> In testing with the patches on top of kernel 6.9.6, setting the >>> property to `Require color accuracy` makes the sysfs file correctly >>> report "Device or resource busy" when trying to change the power >>> saving level, but setting the property to zero doesn't really work. >>> Once KWin sets the property to zero, changing the power saving level >>> "works" but the screen blanks for a moment (might just be a single >>> frame) and reading from the file returns zero again, with the visuals >>> and backlight level unchanged as well. -- Jani Nikula, Intel
Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors
On Mon, 01 Jul 2024, Xaver Hugl wrote: > Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl : >> Merging can only happen once a real world userspace application has >> implemented support for it. I'll try to do that sometime next week in >> KWin > > Here's the promised implementation: > https://invent.kde.org/plasma/kwin/-/merge_requests/6028 The requirement is that the userspace patches must be reviewed and ready for merging into a suitable and canonical upstream project. Are they? BR, Jani. > > In testing with the patches on top of kernel 6.9.6, setting the > property to `Require color accuracy` makes the sysfs file correctly > report "Device or resource busy" when trying to change the power > saving level, but setting the property to zero doesn't really work. > Once KWin sets the property to zero, changing the power saving level > "works" but the screen blanks for a moment (might just be a single > frame) and reading from the file returns zero again, with the visuals > and backlight level unchanged as well. -- Jani Nikula, Intel
Re: [PATCH 2/9] drm/i915: Use backlight power constants
On Wed, 31 Jul 2024, Thomas Zimmermann wrote: > Hi > > Am 31.07.24 um 14:56 schrieb Jani Nikula: >> On Wed, 31 Jul 2024, Thomas Zimmermann wrote: >>> Replace FB_BLANK_ constants with their counterparts from the >>> backlight subsystem. The values are identical, so there's no >>> change in functionality or semantics. >>> >>> Signed-off-by: Thomas Zimmermann >>> Cc: Jani Nikula >>> Cc: Rodrigo Vivi >>> Cc: Joonas Lahtinen >>> Cc: Tvrtko Ursulin >> Reviewed-by: Jani Nikula > > Thanks. > >> >> Do you want us to take this via drm-intel-next, or all together via >> drm-misc? Either is fine. > > drm-intel-next is fine. Pushed to drm-intel-next, thanks for the patch. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v3 0/2] drm: minimum backlight overrides and implementation for amdgpu
On Wed, 31 Jul 2024, Thomas Weißschuh wrote: > The value of "min_input_signal" returned from ATIF on a Framework AMD 13 > is "12". This leads to a fairly bright minimum display backlight. > > Add a generic override helper for the user to override the settings > provided by the firmware through the kernel cmdline. > Also add amdgpu as a user of that helper. > > One solution would be a fixed firmware version, which was announced but > has no timeline. The flip side is that if we add this now, it pretty much has a timeline: We'll have to carry and support it forever. It's not a great prospect for something so specific. Not to mention that the limits are generally there for electrical minimums that should not be overridden. And before you know it, we'll have bug reports about flickering screens... BR, Jani. > > This helper does conflict with the mode override via the cmdline. > Only one can be specified. > IMO the mode override can be extended to also handle "min-brightness" > when that becomes necessary. > > --- > Changes in v3: > - Switch to cmdline override parameter > - Link to v2: > https://lore.kernel.org/r/20240623-amdgpu-min-backlight-quirk-v2-0-cecf7f49d...@weissschuh.net > > Changes in v2: > - Introduce proper drm backlight quirk infrastructure > - Quirk by EDID and DMI instead of only DMI > - Limit quirk to only single Framework 13 matte panel > - Link to v1: > https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5...@weissschuh.net > > --- > Thomas Weißschuh (2): > drm/connector: add drm_connector_get_cmdline_min_brightness_override() > drm/amd/display: implement minimum brightness override > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 > drivers/gpu/drm/drm_connector.c | 34 > +++ > include/drm/drm_connector.h | 2 ++ > 3 files changed, 42 insertions(+) > --- > base-commit: 36821612eb3091a21f7f4a907b497064725080c3 > change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a > > Best regards, -- Jani Nikula, Intel
Re: [PATCH 2/9] drm/i915: Use backlight power constants
On Wed, 31 Jul 2024, Thomas Zimmermann wrote:
> Replace FB_BLANK_ constants with their counterparts from the
> backlight subsystem. The values are identical, so there's no
> change in functionality or semantics.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Jani Nikula
> Cc: Rodrigo Vivi
> Cc: Joonas Lahtinen
> Cc: Tvrtko Ursulin
Reviewed-by: Jani Nikula
Do you want us to take this via drm-intel-next, or all together via
drm-misc? Either is fine.
> ---
> drivers/gpu/drm/i915/display/intel_backlight.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
> b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 071668bfe5d1..6f678c039ed8 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -455,7 +455,7 @@ void intel_backlight_disable(const struct
> drm_connector_state *old_conn_state)
> mutex_lock(&i915->display.backlight.lock);
>
> if (panel->backlight.device)
> - panel->backlight.device->props.power = FB_BLANK_POWERDOWN;
> + panel->backlight.device->props.power = BACKLIGHT_POWER_OFF;
> panel->backlight.enabled = false;
> panel->backlight.funcs->disable(old_conn_state, 0);
>
> @@ -773,7 +773,7 @@ static void __intel_backlight_enable(const struct
> intel_crtc_state *crtc_state,
> panel->backlight.funcs->enable(crtc_state, conn_state,
> panel->backlight.level);
> panel->backlight.enabled = true;
> if (panel->backlight.device)
> - panel->backlight.device->props.power = FB_BLANK_UNBLANK;
> + panel->backlight.device->props.power = BACKLIGHT_POWER_ON;
> }
>
> void intel_backlight_enable(const struct intel_crtc_state *crtc_state,
> @@ -870,12 +870,12 @@ static int intel_backlight_device_update_status(struct
> backlight_device *bd)
>*/
> if (panel->backlight.enabled) {
> if (panel->backlight.power) {
> - bool enable = bd->props.power == FB_BLANK_UNBLANK &&
> + bool enable = bd->props.power == BACKLIGHT_POWER_ON &&
> bd->props.brightness != 0;
> panel->backlight.power(connector, enable);
> }
> } else {
> - bd->props.power = FB_BLANK_POWERDOWN;
> + bd->props.power = BACKLIGHT_POWER_OFF;
> }
>
> drm_modeset_unlock(&i915->drm.mode_config.connection_mutex);
> @@ -945,9 +945,9 @@ int intel_backlight_device_register(struct
> intel_connector *connector)
> props.max_brightness);
>
> if (panel->backlight.enabled)
> - props.power = FB_BLANK_UNBLANK;
> + props.power = BACKLIGHT_POWER_ON;
> else
> - props.power = FB_BLANK_POWERDOWN;
> + props.power = BACKLIGHT_POWER_OFF;
>
> name = kstrdup_const("intel_backlight", GFP_KERNEL);
> if (!name)
--
Jani Nikula, Intel
Re: [PATCH] drm/edid: add CTA Video Format Data Block support
lock_rate / 1000;
> + mode->hdisplay = rid->hactive;
> + mode->hsync_start = htotal - OVT_HSYNC_WIDTH * 2;
> + mode->hsync_end = mode->hsync_start + OVT_HSYNC_WIDTH;
> + mode->htotal = htotal;
> +
> + mode->vdisplay = rid->vactive;
> + mode->vsync_start = vtotal - vsync_position;
> + mode->vsync_end = mode->vsync_start + OVT_VSYNC_WIDTH;
> + mode->vtotal = vtotal;
> +
> + return mode;
> +}
> +
> +/* CTA-861 Video Format Data Block (CTA VFDB) */
> +static void parse_cta_vfdb(struct drm_connector *connector,
> +const struct cea_db *db)
> +{
> + struct drm_display_info *info = &connector->display_info;
> + int vfdb_len = cea_db_payload_len(db);
> + int vfd_len = (db->data[0] & 0x3) + 1;
What if payload len is 0?
> + struct drm_display_mode **modes;
> + struct drm_display_mode *mode;
> + struct cta_vfd vfd;
> + int mode_index = 0;
> + int i;
> + int j;
> +
> + if (!(vfdb_len - 1) || (vfdb_len - 1) % vfd_len)
> + return;
Better to check for vfd_len < some minimum.
I'd usually not require the modulo is zero, just take as many whole
vfd's as there are, and ignore the rest.
> +
> + modes = krealloc_array(info->ovt_modes, ((vfdb_len - 1) / vfd_len) *
> +(ARRAY_SIZE(cta_vf_fr) - 1),
> +sizeof(*info->ovt_modes), GFP_KERNEL);
> +
I really hope we can get rid of this.
> + if (!modes)
> + return;
> +
> + for (i = 1; i < vfdb_len; i += vfd_len) {
> + parse_cta_vfd(&db->data[i], vfd_len, &vfd);
> +
> + if (!vfd.rid || vfd.rid >= ARRAY_SIZE(rids))
> + continue;
> +
> + for (j = 1; j < ARRAY_SIZE(cta_vf_fr); j++) {
> + if (!vfd_has_fr(&vfd, j) ||
> + (cta_vf_fr[j] < 144 && rid_to_vic[vfd.rid][j - 1]))
> + continue;
> +
> + mode = calculate_ovt_mode(&rids[vfd.rid], cta_vf_fr[j],
> + connector->dev);
> +
> + if (!mode)
> + continue;
> +
> + mode->height_mm = info->height_mm;
> + mode->width_mm = info->width_mm;
> +
> + info->ovt_modes[mode_index++] = mode;
> + }
> + }
> +
> + info->num_ovt_modes = mode_index;
> +}
> +
> /*
> * Update y420_cmdb_modes based on previously parsed CTA VDB and Y420CMDB.
> *
> @@ -6439,6 +6839,8 @@ static void drm_parse_cea_ext(struct drm_connector
> *connector,
> parse_cta_vdb(connector, db);
> else if (cea_db_tag(db) == CTA_DB_AUDIO)
> info->has_audio = true;
> + else if (cea_db_tag(db) == CTA_DB_VIDEO_FORMAT)
> + parse_cta_vfdb(connector, db);
> }
> cea_db_iter_end(&iter);
>
> @@ -6585,6 +6987,7 @@ static void drm_update_mso(struct drm_connector
> *connector,
> static void drm_reset_display_info(struct drm_connector *connector)
> {
> struct drm_display_info *info = &connector->display_info;
> + int i;
>
> info->width_mm = 0;
> info->height_mm = 0;
> @@ -6611,6 +7014,13 @@ static void drm_reset_display_info(struct
> drm_connector *connector)
> info->mso_pixel_overlap = 0;
> info->max_dsc_bpp = 0;
>
> + for (i = 0; i < info->num_ovt_modes; i++)
> + drm_mode_destroy(connector->dev, info->ovt_modes[i]);
> +
> + kfree(info->ovt_modes);
> + info->ovt_modes = NULL;
> + info->num_ovt_modes = 0;
> +
I really hope we can get rid of this.
> kfree(info->vics);
> info->vics = NULL;
> info->vics_len = 0;
> @@ -6849,6 +7259,21 @@ static int add_displayid_detailed_modes(struct
> drm_connector *connector,
> return num_modes;
> }
>
> +static int add_ovt_modes(struct drm_connector *connector)
> +{
> + struct drm_display_info *info = &connector->display_info;
> + int i;
> +
> + for (i = 0; i < info->num_ovt_modes; i++) {
> + drm_mode_probed_add(connector, info->ovt_modes[i]);
> + info->ovt_modes[i] = NULL;
> + }
> +
> + info->num_ovt_modes = 0;
> +
> + return i;
> +}
> +
> static int _drm_edid_connector_add_modes(struct drm_connector *connector,
>const struct drm_edid *drm_edid)
> {
> @@ -6872,6 +7297,7 @@ static int _drm_edid_connector_add_modes(struct
> drm_connector *connector,
>*
>* XXX order for additional mode types in extension blocks?
>*/
> + num_modes += add_ovt_modes(connector);
Why first?
> num_modes += add_detailed_modes(connector, drm_edid);
> num_modes += add_cvt_modes(connector, drm_edid);
> num_modes += add_standard_modes(connector, drm_edid);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5ad735253413..35b5eb344ea8 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -829,6 +829,18 @@ struct drm_display_info {
>*/
> u32 max_dsc_bpp;
>
> + /**
> + * @ovt_modes: Array of @num_ovt_modes OVT modes. Internal to EDID
> + * parsing.
> + */
> + struct drm_display_mode **ovt_modes;
> +
> + /**
> + * @num_ovt_modes: Number of elements in @ovt_modes. Internal to EDID
> + * parsing.
> + */
> + int num_ovt_modes;
> +
I really hope we can get rid of this.
> /**
>* @vics: Array of vics_len VICs. Internal to EDID parsing.
>*/
--
Jani Nikula, Intel
Re: [PATCH v2 2/2] drm/radeon: convert bios_hardcoded_edid to drm_edid
On Fri, 26 Jul 2024, Alex Deucher wrote: > Applied the series. Thanks! Ah, replied to patch 1 before noticing this. Never mind about the bikeshedding. :) BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v2 1/2] drm/amdgpu: convert bios_hardcoded_edid to drm_edid
edid_size;
>
> if
> (fake_edid_record->ucFakeEDIDLength == 128)
> edid_size =
> fake_edid_record->ucFakeEDIDLength;
> else
> edid_size =
> fake_edid_record->ucFakeEDIDLength * 128;
> - edid =
> kmemdup(&fake_edid_record->ucFakeEDIDString[0],
> -edid_size,
> GFP_KERNEL);
> - if (edid) {
> - if
> (drm_edid_is_valid(edid)) {
> -
> adev->mode_info.bios_hardcoded_edid = edid;
> -
> adev->mode_info.bios_hardcoded_edid_size = edid_size;
> - } else {
> - kfree(edid);
> - }
> - }
> + edid =
> drm_edid_alloc(fake_edid_record->ucFakeEDIDString, edid_size);
> + if (drm_edid_valid(edid))
> +
> adev->mode_info.bios_hardcoded_edid = edid;
> + else
> + drm_edid_free(edid);
> record +=
> struct_size(fake_edid_record,
>
> ucFakeEDIDString,
>
> edid_size);
It also makes review easier because you don't have to check what goes on
outside of the patch context here. It just won't build.
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index dddb5fe16f2c..742adbc460c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2846,7 +2846,7 @@ static int dce_v10_0_sw_fini(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> - kfree(adev->mode_info.bios_hardcoded_edid);
> + drm_edid_free(adev->mode_info.bios_hardcoded_edid);
>
> drm_kms_helper_poll_fini(adev_to_drm(adev));
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index 11780e4d7e9f..8d46ebadfa46 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2973,7 +2973,7 @@ static int dce_v11_0_sw_fini(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> - kfree(adev->mode_info.bios_hardcoded_edid);
> + drm_edid_free(adev->mode_info.bios_hardcoded_edid);
>
> drm_kms_helper_poll_fini(adev_to_drm(adev));
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index 05c0df97f01d..f08dc6a3886f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -2745,7 +2745,7 @@ static int dce_v6_0_sw_fini(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> - kfree(adev->mode_info.bios_hardcoded_edid);
> + drm_edid_free(adev->mode_info.bios_hardcoded_edid);
>
> drm_kms_helper_poll_fini(adev_to_drm(adev));
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index dc73e301d937..a6a3adf2ae13 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -2766,7 +2766,7 @@ static int dce_v8_0_sw_fini(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> - kfree(adev->mode_info.bios_hardcoded_edid);
> + drm_edid_free(adev->mode_info.bios_hardcoded_edid);
>
> drm_kms_helper_poll_fini(adev_to_drm(adev));
--
Jani Nikula, Intel
Re: [PATCH v7 1/2] drm/buddy: Add start address support to trim function
ew(struct
> ttm_resource_manager *man,
> } while (remaining_size);
>
> if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
> - if (!drm_buddy_block_trim(mm, vres->base.size, &vres->blocks))
> + if (!drm_buddy_block_trim(mm, NULL, vres->base.size,
> &vres->blocks))
> size = vres->base.size;
> }
>
> diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
> index 2a74fa9d0ce5..9689a7c5dd36 100644
> --- a/include/drm/drm_buddy.h
> +++ b/include/drm/drm_buddy.h
> @@ -27,6 +27,7 @@
> #define DRM_BUDDY_CONTIGUOUS_ALLOCATION BIT(2)
> #define DRM_BUDDY_CLEAR_ALLOCATION BIT(3)
> #define DRM_BUDDY_CLEAREDBIT(4)
> +#define DRM_BUDDY_TRIM_DISABLE BIT(5)
>
> struct drm_buddy_block {
> #define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
> @@ -155,6 +156,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
> unsigned long flags);
>
> int drm_buddy_block_trim(struct drm_buddy *mm,
> + u64 *start,
>u64 new_size,
>struct list_head *blocks);
>
>
> base-commit: b27d70e1042bf6a31ba7e5acf58b61c9cd28f95b
--
Jani Nikula, Intel
Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13
rom Framework would be good. But when things do not work 100% adding > a kernel cmdline option is something which is regularly asked from users / > found in support questions on fora so I don't think this is overly > complicated. I agree it is not ideal but IMHO it is workable. > > E.g. on Fedora it would simply be a question of users having to run: > > sudo grubby --update-kernel=ALL --args="video=eDP-1:min-brightness=1" > > will add the passed in argument to all currently installed (and > future) kernels. > >> Some more background to the Framework 13 AMD case: >> The same panel on the Intel variant already goes darker. >> The last responses we got from Framework didn't indicate that the high >> minimum brightness was intentional [0], [1]. >> Coincidentally the "12" returned from ATIF matches >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set >> up completely. > > Right, so I think this should be investigated closer and then get > framework to issue a BIOS fix, not add a quirk mechanism to the kernel. > > IIRC the amdgpu driver will use AMDGPU_DM_DEFAULT_MIN_BACKLIGHT when > that setting is 0 in the VBT. > >> >>> The minimum brightness override set this way will still need hooking up >>> in each driver separately but by using the video=eDP-1:... mechanism >>> we can document how to do this in driver independent manner. since >>> I know there have been multiple requests for something like this in >>> the past I believe that having a single uniform way for users to do this >>> will be good. >>> >>> Alternatively we could have each driver have a driver specific module- >>> parameter for this. Either way I think we need some way for users to >>> override this as a config/setting tweak rather then use quirks for this. >> >> This also seems much too complicated for normal users. > > I agree that having a uniform way is better then having per driver > module options. > > Regards, > > Hans > -- Jani Nikula, Intel
Re: [PATCH v7 2/9] drm: Support per-plane async flip configuration
On Tue, 18 Jun 2024, André Almeida wrote:
> Drivers have different capabilities on what plane types they can or
> cannot perform async flips. Create a plane::async_flip field so each
> driver can choose which planes they allow doing async flips.
>
> Signed-off-by: André Almeida
> ---
> include/drm/drm_plane.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9507542121fa..0bebc72af5c3 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -786,6 +786,11 @@ struct drm_plane {
>* @kmsg_panic: Used to register a panic notifier for this plane
>*/
> struct kmsg_dumper kmsg_panic;
> +
> + /**
> + * @async_flip: indicates if a plane can do async flips
> + */
When is it okay to set or change the value of this member?
If you don't document it, people will find creative uses for this.
BR,
Jani.
> + bool async_flip;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
--
Jani Nikula, Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, 05 Jun 2024, Chris Bainbridge wrote:
> On Tue, Jun 04, 2024 at 10:02:29AM +0800, kernel test robot wrote:
>> Hi Mario,
>>
>> kernel test robot noticed the following build errors:
>>
>> [auto build test ERROR on drm-misc/drm-misc-next]
>> [also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next
>> drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip
>> linus/master v6.10-rc2 next-20240603]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:
>> https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/drm-client-Detect-when-ACPI-lid-is-closed-during-initialization/20240529-050440
>> base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
>> patch link:
>> https://lore.kernel.org/r/20240528210319.1242-1-mario.limonciello%40amd.com
>> patch subject: [PATCH v2] drm/client: Detect when ACPI lid is closed during
>> initialization
>> config: i386-randconfig-053-20240604
>> (https://download.01.org/0day-ci/archive/20240604/202406040928.eu1griwv-...@intel.com/config)
>> compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0
>> reproduce (this is a W=1 build):
>> (https://download.01.org/0day-ci/archive/20240604/202406040928.eu1griwv-...@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version
>> of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot
>> | Closes:
>> https://lore.kernel.org/oe-kbuild-all/202406040928.eu1griwv-...@intel.com/
>>
>> All errors (new ones prefixed by >>):
>>
>>ld: drivers/gpu/drm/drm_client_modeset.o: in function
>> `drm_client_match_edp_lid':
>> >> drivers/gpu/drm/drm_client_modeset.c:281:(.text+0x221b): undefined
>> >> reference to `acpi_lid_open'
>>
>>
>> vim +281 drivers/gpu/drm/drm_client_modeset.c
>>
>>260
>>261 static void drm_client_match_edp_lid(struct drm_device *dev,
>>262struct drm_connector
>> **connectors,
>>263unsigned int
>> connector_count,
>>264bool *enabled)
>>265 {
>>266 int i;
>>267
>>268 for (i = 0; i < connector_count; i++) {
>>269 struct drm_connector *connector = connectors[i];
>>270
>>271 switch (connector->connector_type) {
>>272 case DRM_MODE_CONNECTOR_LVDS:
>>273 case DRM_MODE_CONNECTOR_eDP:
>>274 if (!enabled[i])
>>275 continue;
>>276 break;
>>277 default:
>>278 continue;
>>279 }
>>280
>> > 281 if (!acpi_lid_open()) {
>>282 drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid
>> is closed, disabling\n",
>>283 connector->base.id,
>> connector->name);
>>284 enabled[i] = false;
>>285 }
>>286 }
>>287 }
>>288
>>
>> --
>> 0-DAY CI Kernel Test Service
>> https://github.com/intel/lkp-tests/wiki
>
> The failed config has CONFIG_ACPI_BUTTON=m. The build failure can be
> fixed with:
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c
> b/drivers/gpu/drm/drm_client_modeset.c
> index b76438c31761..0271e66f44f8 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -271,11 +271,13 @@ static void drm_client_match_edp_lid(struct drm_device
> *dev,
> if (connector->connector_type != DRM_MODE_CONNECTOR_eDP ||
> !enabled[i])
> continue;
>
> +#if defined(CONFIG_ACPI_BUTTON)
> if (!acpi_lid_open()) {
> drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed,
> disabling\n",
> connector->base.id, connector->name);
> enabled[i] = false;
> }
> +#endif
> }
> }
No. This is because
CONFIG_DRM=y
CONFIG_ACPI_BUTTON=m
The pedantically correct fix is probably having DRM
depends on ACPI_BUTTON || ACPI_BUTTON=n
but seeing how i915 and xe just
select ACPI_BUTTON if ACPI
and nouveau basically uses acpi_lid_open() it if it's reachable with no
regard to kconfig, it's anyone's guess what will work.
BR,
Jani.
--
Jani Nikula, Intel
Re: [PATCH 2/3] drm/xe: drop redundant W=1 warnings from Makefile
On Thu, 23 May 2024, Jani Nikula wrote:
> Since commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default
> across the subsystem"), most of the extra warnings in the driver
> Makefile are redundant. Remove them.
>
> Note that -Wmissing-declarations and -Wmissing-prototypes are always
> enabled by default in scripts/Makefile.extrawarn.
>
> Signed-off-by: Jani Nikula
Pushed this patch to drm-xe-next with Lucas' irc ack.
BR,
Jani.
> ---
> drivers/gpu/drm/xe/Makefile | 25 +
> 1 file changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index c9f067b8f54d..f4366cb958be 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -3,31 +3,8 @@
> # Makefile for the drm device driver. This driver provides support for the
> # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
> -# Unconditionally enable W=1 warnings locally
> -# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn
> -subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter
> -subdir-ccflags-y += -Wmissing-declarations
> -subdir-ccflags-y += $(call cc-option, -Wrestrict)
> -subdir-ccflags-y += -Wmissing-format-attribute
> -subdir-ccflags-y += -Wmissing-prototypes
> -subdir-ccflags-y += -Wold-style-definition
> -subdir-ccflags-y += -Wmissing-include-dirs
> -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable)
> -subdir-ccflags-y += $(call cc-option, -Wunused-const-variable)
> -subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned)
> -subdir-ccflags-y += $(call cc-option, -Wformat-overflow)
> +# Enable W=1 warnings not enabled in drm subsystem Makefile
> subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
> -subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
> -# The following turn off the warnings enabled by -Wextra
> -ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
> -subdir-ccflags-y += -Wno-missing-field-initializers
> -subdir-ccflags-y += -Wno-type-limits
> -subdir-ccflags-y += -Wno-shift-negative-value
> -endif
> -ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),)
> -subdir-ccflags-y += -Wno-sign-compare
> -endif
> -# --- end copy-paste
>
> # Enable -Werror in CI and development
> subdir-ccflags-$(CONFIG_DRM_XE_WERROR) += -Werror
--
Jani Nikula, Intel
Re: [PATCH 3/3] drm/amdgpu: drop redundant W=1 warnings from Makefile
On Thu, 23 May 2024, Jani Nikula wrote:
> Since commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default
> across the subsystem"), most of the extra warnings in the driver
> Makefile are redundant. Remove them.
>
> Note that -Wmissing-declarations and -Wmissing-prototypes are always
> enabled by default in scripts/Makefile.extrawarn.
>
> Signed-off-by: Jani Nikula
Alex, this one's for you to do whatever you want. ;)
BR,
Jani.
> ---
> drivers/gpu/drm/amd/amdgpu/Makefile | 18 +-
> 1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 1f6b56ec99f6..9508d0b5708e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -39,23 +39,7 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \
> -I$(FULL_AMD_DISPLAY_PATH)/amdgpu_dm \
> -I$(FULL_AMD_PATH)/amdkfd
>
> -subdir-ccflags-y := -Wextra
> -subdir-ccflags-y += -Wunused
> -subdir-ccflags-y += -Wmissing-prototypes
> -subdir-ccflags-y += -Wmissing-declarations
> -subdir-ccflags-y += -Wmissing-include-dirs
> -subdir-ccflags-y += -Wold-style-definition
> -subdir-ccflags-y += -Wmissing-format-attribute
> -# Need this to avoid recursive variable evaluation issues
> -cond-flags := $(call cc-option, -Wunused-but-set-variable) \
> - $(call cc-option, -Wunused-const-variable) \
> - $(call cc-option, -Wstringop-truncation) \
> - $(call cc-option, -Wpacked-not-aligned)
> -subdir-ccflags-y += $(cond-flags)
> -subdir-ccflags-y += -Wno-unused-parameter
> -subdir-ccflags-y += -Wno-type-limits
> -subdir-ccflags-y += -Wno-sign-compare
> -subdir-ccflags-y += -Wno-missing-field-initializers
> +# Locally disable W=1 warnings enabled in drm subsystem Makefile
> subdir-ccflags-y += -Wno-override-init
> subdir-ccflags-$(CONFIG_DRM_AMDGPU_WERROR) += -Werror
--
Jani Nikula, Intel
Re: [PATCH 1/3] drm/i915: drop redundant W=1 warnings from Makefile
On Thu, 23 May 2024, Jani Nikula wrote:
> Since commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default
> across the subsystem"), most of the extra warnings in the driver
> Makefile are redundant. Remove them.
>
> Note that -Wmissing-declarations and -Wmissing-prototypes are always
> enabled by default in scripts/Makefile.extrawarn.
>
> Signed-off-by: Jani Nikula
Pushed this patch to drm-intel-next with Lucas' irc ack.
BR,
Jani.
> ---
> drivers/gpu/drm/i915/Makefile | 25 +
> 1 file changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 7cad944b825c..a70d95a8fd7a 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -3,31 +3,8 @@
> # Makefile for the drm device driver. This driver provides support for the
> # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
> -# Unconditionally enable W=1 warnings locally
> -# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn
> -subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter
> -subdir-ccflags-y += -Wmissing-declarations
> -subdir-ccflags-y += $(call cc-option, -Wrestrict)
> -subdir-ccflags-y += -Wmissing-format-attribute
> -subdir-ccflags-y += -Wmissing-prototypes
> -subdir-ccflags-y += -Wold-style-definition
> -subdir-ccflags-y += -Wmissing-include-dirs
> -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable)
> -subdir-ccflags-y += $(call cc-option, -Wunused-const-variable)
> -subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned)
> -subdir-ccflags-y += $(call cc-option, -Wformat-overflow)
> +# Enable W=1 warnings not enabled in drm subsystem Makefile
> subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
> -subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
> -# The following turn off the warnings enabled by -Wextra
> -ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
> -subdir-ccflags-y += -Wno-missing-field-initializers
> -subdir-ccflags-y += -Wno-type-limits
> -subdir-ccflags-y += -Wno-shift-negative-value
> -endif
> -ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),)
> -subdir-ccflags-y += -Wno-sign-compare
> -endif
> -# --- end copy-paste
>
> # Enable -Werror in CI and development
> subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
--
Jani Nikula, Intel
[PATCH 0/3] drm/mst & drm/amd/display: switch to using guid_t
We have a guid_t type for GUIDs, switch to using it instead of hand rolling buffers. Convert to guid_gen() in separate patches to pinpoint the functional changes. BR, Jani. Jani Nikula (3): drm/mst: switch to guid_t type for GUID drm/mst: switch to guid_gen() to generate valid GUIDs drm/amd/display: switch to guid_gen() to generate valid GUIDs .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 67 ++- include/drm/display/drm_dp_mst_helper.h | 12 ++-- 3 files changed, 52 insertions(+), 50 deletions(-) -- 2.39.2
[PATCH 3/3] drm/amd/display: switch to guid_gen() to generate valid GUIDs
Instead of just smashing jiffies into a GUID, use guid_gen() to generate
RFC 4122 compliant GUIDs.
Signed-off-by: Jani Nikula
---
Side note, it baffles me why amdgpu has a copy of this instead of
plumbing it into drm mst code.
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++-
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 65ebc01dc90f..a1bd847857b8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2403,9 +2403,9 @@ static int dm_late_init(void *handle)
static void resume_mst_branch_status(struct drm_dp_mst_topology_mgr *mgr)
{
+ u8 buf[UUID_SIZE];
+ guid_t guid;
int ret;
- u8 guid[16];
- u64 tmp64;
mutex_lock(&mgr->lock);
if (!mgr->mst_primary)
@@ -2426,26 +2426,27 @@ static void resume_mst_branch_status(struct
drm_dp_mst_topology_mgr *mgr)
}
/* Some hubs forget their guids after they resume */
- ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
- if (ret != 16) {
+ ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, buf, sizeof(buf));
+ if (ret != sizeof(buf)) {
drm_dbg_kms(mgr->dev, "dpcd read failed - undocked during
suspend?\n");
goto out_fail;
}
- if (memchr_inv(guid, 0, 16) == NULL) {
- tmp64 = get_jiffies_64();
- memcpy(&guid[0], &tmp64, sizeof(u64));
- memcpy(&guid[8], &tmp64, sizeof(u64));
+ import_guid(&guid, buf);
- ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, guid, 16);
+ if (guid_is_null(&guid)) {
+ guid_gen(&guid);
+ export_guid(buf, &guid);
- if (ret != 16) {
+ ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, buf, sizeof(buf));
+
+ if (ret != sizeof(buf)) {
drm_dbg_kms(mgr->dev, "check mstb guid failed -
undocked during suspend?\n");
goto out_fail;
}
}
- import_guid(&mgr->mst_primary->guid, guid);
+ guid_copy(&mgr->mst_primary->guid, &guid);
out_fail:
mutex_unlock(&mgr->lock);
--
2.39.2
[PATCH 2/3] drm/mst: switch to guid_gen() to generate valid GUIDs
Instead of just smashing jiffies into a GUID, use guid_gen() to generate
RFC 4122 compliant GUIDs.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 9b1f35b1a2da..1cb071daab8f 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2698,18 +2698,10 @@ static void drm_dp_mst_link_probe_work(struct
work_struct *work)
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
guid_t *guid)
{
- u64 salt;
- u8 buf[UUID_SIZE];
-
if (!guid_is_null(guid))
return true;
- salt = get_jiffies_64();
-
- memcpy(&buf[0], &salt, sizeof(u64));
- memcpy(&buf[8], &salt, sizeof(u64));
-
- import_guid(guid, buf);
+ guid_gen(guid);
return false;
}
--
2.39.2
[PATCH 1/3] drm/mst: switch to guid_t type for GUID
The kernel has a guid_t type for GUIDs. Switch to using it, but avoid
any functional changes here.
Signed-off-by: Jani Nikula
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
drivers/gpu/drm/display/drm_dp_mst_topology.c | 67 +++
include/drm/display/drm_dp_mst_helper.h | 12 ++--
3 files changed, 45 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 516eb3968e26..65ebc01dc90f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2445,7 +2445,7 @@ static void resume_mst_branch_status(struct
drm_dp_mst_topology_mgr *mgr)
}
}
- memcpy(mgr->mst_primary->guid, guid, 16);
+ import_guid(&mgr->mst_primary->guid, guid);
out_fail:
mutex_unlock(&mgr->lock);
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 7f8e1cfbe19d..9b1f35b1a2da 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -89,7 +89,7 @@ static int drm_dp_send_enum_path_resources(struct
drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_branch *mstb,
struct drm_dp_mst_port *port);
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
-u8 *guid);
+guid_t *guid);
static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port);
static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port);
@@ -801,7 +801,7 @@ static bool drm_dp_sideband_parse_link_address(const struct
drm_dp_mst_topology_
int idx = 1;
int i;
- memcpy(repmsg->u.link_addr.guid, &raw->msg[idx], 16);
+ import_guid(&repmsg->u.link_addr.guid, &raw->msg[idx]);
idx += 16;
repmsg->u.link_addr.nports = raw->msg[idx] & 0xf;
idx++;
@@ -829,7 +829,7 @@ static bool drm_dp_sideband_parse_link_address(const struct
drm_dp_mst_topology_
idx++;
if (idx > raw->curlen)
goto fail_len;
- memcpy(repmsg->u.link_addr.ports[i].peer_guid,
&raw->msg[idx], 16);
+ import_guid(&repmsg->u.link_addr.ports[i].peer_guid,
&raw->msg[idx]);
idx += 16;
if (idx > raw->curlen)
goto fail_len;
@@ -1029,7 +1029,7 @@ static bool drm_dp_sideband_parse_reply(const struct
drm_dp_mst_topology_mgr *mg
msg->req_type = (raw->msg[0] & 0x7f);
if (msg->reply_type == DP_SIDEBAND_REPLY_NAK) {
- memcpy(msg->u.nak.guid, &raw->msg[1], 16);
+ import_guid(&msg->u.nak.guid, &raw->msg[1]);
msg->u.nak.reason = raw->msg[17];
msg->u.nak.nak_data = raw->msg[18];
return false;
@@ -1078,7 +1078,7 @@ drm_dp_sideband_parse_connection_status_notify(const
struct drm_dp_mst_topology_
if (idx > raw->curlen)
goto fail_len;
- memcpy(msg->u.conn_stat.guid, &raw->msg[idx], 16);
+ import_guid(&msg->u.conn_stat.guid, &raw->msg[idx]);
idx += 16;
if (idx > raw->curlen)
goto fail_len;
@@ -1107,7 +1107,7 @@ static bool
drm_dp_sideband_parse_resource_status_notify(const struct drm_dp_mst
if (idx > raw->curlen)
goto fail_len;
- memcpy(msg->u.resource_stat.guid, &raw->msg[idx], 16);
+ import_guid(&msg->u.resource_stat.guid, &raw->msg[idx]);
idx += 16;
if (idx > raw->curlen)
goto fail_len;
@@ -2174,20 +2174,24 @@ ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
offset, size, buffer);
}
-static int drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid)
+static int drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, guid_t *guid)
{
int ret = 0;
- memcpy(mstb->guid, guid, 16);
+ guid_copy(&mstb->guid, guid);
+
+ if (!drm_dp_validate_guid(mstb->mgr, &mstb->guid)) {
+ u8 buf[UUID_SIZE];
+
+ export_guid(buf, &mstb->guid);
- if (!drm_dp_validate_guid(mstb->mgr, mstb->guid)) {
if (mstb->port_parent) {
ret = drm_dp_send_dpcd_write(mstb->mgr,
mstb->port_parent,
-DP_GUID, 16
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, 29 May 2024, Alex Deucher wrote:
> On Tue, May 28, 2024 at 5:03 PM Mario Limonciello
> wrote:
>>
>> If the lid on a laptop is closed when eDP connectors are populated
>> then it remains enabled when the initial framebuffer configuration
>> is built.
>>
>> When creating the initial framebuffer configuration detect the ACPI
>> lid status and if it's closed disable any eDP connectors.
>>
>> Reported-by: Chris Bainbridge
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
>> Signed-off-by: Mario Limonciello
>
> Reviewed-by: Alex Deucher
>
> Do you have drm-misc access or do you need someone to apply this for you?
I've bounced this to intel-gfx and intel-xe lists to get CI testing. I'd
appreciate holding off on merging until we have results.
Thanks,
Jani.
>
> Alex
>
>> ---
>> Cc: hughsi...@gmail.com
>> v1->v2:
>> * Match LVDS as well
>> ---
>> drivers/gpu/drm/drm_client_modeset.c | 30
>> 1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_client_modeset.c
>> b/drivers/gpu/drm/drm_client_modeset.c
>> index 31af5cf37a09..0b0411086e76 100644
>> --- a/drivers/gpu/drm/drm_client_modeset.c
>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> @@ -8,6 +8,7 @@
>> */
>>
>> #include "drm/drm_modeset_lock.h"
>> +#include
>> #include
>> #include
>> #include
>> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct
>> drm_connector **connectors,
>> enabled[i] = drm_connector_enabled(connectors[i], false);
>> }
>>
>> +static void drm_client_match_edp_lid(struct drm_device *dev,
>> +struct drm_connector **connectors,
>> +unsigned int connector_count,
>> +bool *enabled)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < connector_count; i++) {
>> + struct drm_connector *connector = connectors[i];
>> +
>> + switch (connector->connector_type) {
>> + case DRM_MODE_CONNECTOR_LVDS:
>> + case DRM_MODE_CONNECTOR_eDP:
>> + if (!enabled[i])
>> + continue;
>> + break;
>> + default:
>> + continue;
>> + }
>> +
>> + if (!acpi_lid_open()) {
>> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed,
>> disabling\n",
>> + connector->base.id, connector->name);
>> + enabled[i] = false;
>> + }
>> + }
>> +}
>> +
>> static bool drm_client_target_cloned(struct drm_device *dev,
>> struct drm_connector **connectors,
>> unsigned int connector_count,
>> @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev
>> *client, unsigned int width,
>> memset(crtcs, 0, connector_count * sizeof(*crtcs));
>> memset(offsets, 0, connector_count * sizeof(*offsets));
>>
>> + drm_client_match_edp_lid(dev, connectors, connector_count,
>> enabled);
>> if (!drm_client_target_cloned(dev, connectors,
>> connector_count, modes,
>> offsets, enabled, width,
>> height) &&
>> !drm_client_target_preferred(dev, connectors,
>> connector_count, modes,
>> --
>> 2.43.0
>>
--
Jani Nikula, Intel
Re: [PATCH 3/4] drm/imx: fix -Wformat-truncation warning in imx_ldb_probe()
On Thu, 23 May 2024, Ville Syrjälä wrote:
> On Thu, May 23, 2024 at 06:51:08PM +0300, Jani Nikula wrote:
>> Enabling -Wformat-truncation yields the following warning:
>>
>> ../drivers/gpu/drm/imx/ipuv3/imx-ldb.c: In function ‘imx_ldb_probe’:
>> ../drivers/gpu/drm/imx/ipuv3/imx-ldb.c:658:57: error: ‘_sel’ directive
>> output may be truncated writing 4 bytes into a region of size between 3 and
>> 13 [-Werror=format-truncation=]
>> 658 | snprintf(clkname, sizeof(clkname), "di%d_sel", i);
>> | ^~~~
>> ../drivers/gpu/drm/imx/ipuv3/imx-ldb.c:658:17: note: ‘snprintf’ output
>> between 8 and 18 bytes into a destination of size 16
>> 658 | snprintf(clkname, sizeof(clkname), "di%d_sel", i);
>> | ^
>
> If only the compiler could count to three...
I did not try, but apparently using %hhd would hide the issue too:
snprintf(clkname, sizeof(clkname), "di%hhd_sel", i);
BR,
Jani.
>
>>
>> Silence the warning by checking the snprintf() return value.
>>
>> Signed-off-by: Jani Nikula
>>
>> ---
>>
>> Cc: Philipp Zabel
>> Cc: Shawn Guo
>> Cc: Sascha Hauer
>> Cc: Pengutronix Kernel Team
>> Cc: Fabio Estevam
>> Cc: dri-de...@lists.freedesktop.org
>> Cc: i...@lists.linux.dev
>> ---
>> drivers/gpu/drm/imx/ipuv3/imx-ldb.c | 6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
>> b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
>> index 71d70194fcbd..46f779fe60ee 100644
>> --- a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
>> +++ b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
>> @@ -654,8 +654,12 @@ static int imx_ldb_probe(struct platform_device *pdev)
>> */
>> for (i = 0; i < 4; i++) {
>> char clkname[16];
>> +int len;
>> +
>> +len = snprintf(clkname, sizeof(clkname), "di%d_sel", i);
>> +if (len >= sizeof(clkname))
>> + dev_err(dev, "clkname truncated\n");
>>
>> -snprintf(clkname, sizeof(clkname), "di%d_sel", i);
>> imx_ldb->clk_sel[i] = devm_clk_get(imx_ldb->dev, clkname);
>> if (IS_ERR(imx_ldb->clk_sel[i])) {
>> ret = PTR_ERR(imx_ldb->clk_sel[i]);
>> --
>> 2.39.2
--
Jani Nikula, Intel
Re: [PATCH 4/4] drm: enable -Wformat-truncation across the subsystem
On Thu, 23 May 2024, Sam Ravnborg wrote:
> Hi Jani,
>
> On Thu, May 23, 2024 at 06:51:09PM +0300, Jani Nikula wrote:
>> With the -Wformat-truncation warnings fixed, finish the job started in
>> commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default across
>> the subsystem"), and enable that warning too.
>>
>> Signed-off-by: Jani Nikula
>
> When it is enabled for all of drm then the explicit assignments here
> could be dropped I think:
>
> drivers/gpu/drm/i915/Makefile:subdir-ccflags-y += $(call cc-option,
> -Wformat-truncation)
> drivers/gpu/drm/xe/Makefile:subdir-ccflags-y += $(call cc-option,
> -Wformat-truncation)
>
> Just a drive-by comment, I know this patch was mostly for the bots.
Additionally, I didn't want to create any conflicts with [1]. There's no
harm in having the duplication.
BR,
Jani.
[1] https://lore.kernel.org/r/cover.1716471145.git.jani.nik...@intel.com
>
> Sam
>
>>
>> ---
>>
>> Gut feeling says there are more issues, and my configs just don't catch
>> them all, but let's see what the build bots have to say. ;)
>> ---
>> drivers/gpu/drm/Makefile | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 68cc9258ffc4..644613dbedda 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -16,8 +16,7 @@ subdir-ccflags-y += $(call cc-option,
>> -Wunused-but-set-variable)
>> subdir-ccflags-y += $(call cc-option, -Wunused-const-variable)
>> subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned)
>> subdir-ccflags-y += $(call cc-option, -Wformat-overflow)
>> -# FIXME: fix -Wformat-truncation warnings and uncomment
>> -#subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
>> +subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
>> subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
>> # The following turn off the warnings enabled by -Wextra
>> ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
>> --
>> 2.39.2
--
Jani Nikula, Intel
Re: [PATCH 1/4] drm/amdgpu: fix -Wformat-truncation warning in amdgpu_gfx_kiq_init_ring()
On Thu, 23 May 2024, Alex Deucher wrote: > Already fixed with this patch: > https://patchwork.freedesktop.org/patch/594864/ Great, thanks! BR, Jani. -- Jani Nikula, Intel
[PATCH 0/3] amd, i915, xe: drop redundant warnings from driver makefiles
I'm sending these together, as they're related, and almost identical, but I expect them to be merged individually to each driver. BR, Jani. Jani Nikula (3): drm/i915: drop redundant W=1 warnings from Makefile drm/xe: drop redundant W=1 warnings from Makefile drm/amdgpu: drop redundant W=1 warnings from Makefile drivers/gpu/drm/amd/amdgpu/Makefile | 18 +- drivers/gpu/drm/i915/Makefile | 25 + drivers/gpu/drm/xe/Makefile | 25 + 3 files changed, 3 insertions(+), 65 deletions(-) -- 2.39.2
[PATCH 3/3] drm/amdgpu: drop redundant W=1 warnings from Makefile
Since commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default
across the subsystem"), most of the extra warnings in the driver
Makefile are redundant. Remove them.
Note that -Wmissing-declarations and -Wmissing-prototypes are always
enabled by default in scripts/Makefile.extrawarn.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/amd/amdgpu/Makefile | 18 +-
1 file changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 1f6b56ec99f6..9508d0b5708e 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -39,23 +39,7 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \
-I$(FULL_AMD_DISPLAY_PATH)/amdgpu_dm \
-I$(FULL_AMD_PATH)/amdkfd
-subdir-ccflags-y := -Wextra
-subdir-ccflags-y += -Wunused
-subdir-ccflags-y += -Wmissing-prototypes
-subdir-ccflags-y += -Wmissing-declarations
-subdir-ccflags-y += -Wmissing-include-dirs
-subdir-ccflags-y += -Wold-style-definition
-subdir-ccflags-y += -Wmissing-format-attribute
-# Need this to avoid recursive variable evaluation issues
-cond-flags := $(call cc-option, -Wunused-but-set-variable) \
- $(call cc-option, -Wunused-const-variable) \
- $(call cc-option, -Wstringop-truncation) \
- $(call cc-option, -Wpacked-not-aligned)
-subdir-ccflags-y += $(cond-flags)
-subdir-ccflags-y += -Wno-unused-parameter
-subdir-ccflags-y += -Wno-type-limits
-subdir-ccflags-y += -Wno-sign-compare
-subdir-ccflags-y += -Wno-missing-field-initializers
+# Locally disable W=1 warnings enabled in drm subsystem Makefile
subdir-ccflags-y += -Wno-override-init
subdir-ccflags-$(CONFIG_DRM_AMDGPU_WERROR) += -Werror
--
2.39.2
[PATCH 2/4] drm/nouveau: fix -Wformat-truncation warning in nouveau_backlight_init()
Enabling -Wformat-truncation yields the following warning:
../drivers/gpu/drm/nouveau/nouveau_backlight.c: In function
‘nouveau_backlight_init’:
../drivers/gpu/drm/nouveau/nouveau_backlight.c:56:69: error: ‘%d’ directive
output may be truncated writing between 1 and 10 bytes into a region of size 3
[-Werror=format-truncation=]
56 | snprintf(backlight_name, BL_NAME_SIZE,
"nv_backlight%d", nb);
| ^~
In function ‘nouveau_get_backlight_name’,
inlined from ‘nouveau_backlight_init’ at
../drivers/gpu/drm/nouveau/nouveau_backlight.c:351:7:
../drivers/gpu/drm/nouveau/nouveau_backlight.c:56:56: note: directive argument
in the range [1, 2147483647]
56 | snprintf(backlight_name, BL_NAME_SIZE,
"nv_backlight%d", nb);
|^~~~
../drivers/gpu/drm/nouveau/nouveau_backlight.c:56:17: note: ‘snprintf’ output
between 14 and 23 bytes into a destination of size 15
56 | snprintf(backlight_name, BL_NAME_SIZE,
"nv_backlight%d", nb);
|
^~~~
Silence the warning by checking the snprintf() return value.
Signed-off-by: Jani Nikula
---
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: dri-de...@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
---
drivers/gpu/drm/nouveau/nouveau_backlight.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index d47442125fa1..1d77a5f280c5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -49,13 +49,18 @@ nouveau_get_backlight_name(char
backlight_name[BL_NAME_SIZE],
struct nouveau_backlight *bl)
{
const int nb = ida_alloc_max(&bl_ida, 99, GFP_KERNEL);
+ int ret;
if (nb < 0)
return false;
if (nb > 0)
- snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
+ ret = snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d",
nb);
else
- snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
+ ret = snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
+
+ if (ret >= BL_NAME_SIZE)
+ return false;
+
bl->id = nb;
return true;
}
--
2.39.2
[PATCH 0/4] drm: enable -Wformat-truncation
Jani Nikula (4): drm/amdgpu: fix -Wformat-truncation warning in amdgpu_gfx_kiq_init_ring() drm/nouveau: fix -Wformat-truncation warning in nouveau_backlight_init() drm/imx: fix -Wformat-truncation warning in imx_ldb_probe() drm: enable -Wformat-truncation across the subsystem drivers/gpu/drm/Makefile| 3 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 6 -- drivers/gpu/drm/imx/ipuv3/imx-ldb.c | 6 +- drivers/gpu/drm/nouveau/nouveau_backlight.c | 9 +++-- 4 files changed, 17 insertions(+), 7 deletions(-) -- 2.39.2
[PATCH 1/3] drm/i915: drop redundant W=1 warnings from Makefile
Since commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default
across the subsystem"), most of the extra warnings in the driver
Makefile are redundant. Remove them.
Note that -Wmissing-declarations and -Wmissing-prototypes are always
enabled by default in scripts/Makefile.extrawarn.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/Makefile | 25 +
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7cad944b825c..a70d95a8fd7a 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -3,31 +3,8 @@
# Makefile for the drm device driver. This driver provides support for the
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
-# Unconditionally enable W=1 warnings locally
-# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn
-subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter
-subdir-ccflags-y += -Wmissing-declarations
-subdir-ccflags-y += $(call cc-option, -Wrestrict)
-subdir-ccflags-y += -Wmissing-format-attribute
-subdir-ccflags-y += -Wmissing-prototypes
-subdir-ccflags-y += -Wold-style-definition
-subdir-ccflags-y += -Wmissing-include-dirs
-subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable)
-subdir-ccflags-y += $(call cc-option, -Wunused-const-variable)
-subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned)
-subdir-ccflags-y += $(call cc-option, -Wformat-overflow)
+# Enable W=1 warnings not enabled in drm subsystem Makefile
subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
-subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
-# The following turn off the warnings enabled by -Wextra
-ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
-subdir-ccflags-y += -Wno-missing-field-initializers
-subdir-ccflags-y += -Wno-type-limits
-subdir-ccflags-y += -Wno-shift-negative-value
-endif
-ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),)
-subdir-ccflags-y += -Wno-sign-compare
-endif
-# --- end copy-paste
# Enable -Werror in CI and development
subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
--
2.39.2
[PATCH 4/4] drm: enable -Wformat-truncation across the subsystem
With the -Wformat-truncation warnings fixed, finish the job started in
commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default across
the subsystem"), and enable that warning too.
Signed-off-by: Jani Nikula
---
Gut feeling says there are more issues, and my configs just don't catch
them all, but let's see what the build bots have to say. ;)
---
drivers/gpu/drm/Makefile | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 68cc9258ffc4..644613dbedda 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,8 +16,7 @@ subdir-ccflags-y += $(call cc-option,
-Wunused-but-set-variable)
subdir-ccflags-y += $(call cc-option, -Wunused-const-variable)
subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned)
subdir-ccflags-y += $(call cc-option, -Wformat-overflow)
-# FIXME: fix -Wformat-truncation warnings and uncomment
-#subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
+subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
# The following turn off the warnings enabled by -Wextra
ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
--
2.39.2
[PATCH 1/4] drm/amdgpu: fix -Wformat-truncation warning in amdgpu_gfx_kiq_init_ring()
Enabling -Wformat-truncation yields the following warning: ../drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c: In function ‘amdgpu_gfx_kiq_init_ring’: ../drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:61: error: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size between 0 and 8 [-Werror=format-truncation=] 332 | snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", | ^~ ../drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:50: note: directive argument in the range [0, 2147483647] 332 | snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", | ^ ../drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:9: note: ‘snprintf’ output between 12 and 41 bytes into a destination of size 16 332 | snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", | ^~~ 333 | xcc_id, ring->me, ring->pipe, ring->queue); | ~~ Silence the warning by checking the snprintf() return value. Signed-off-by: Jani Nikula --- Cc: Alex Deucher Cc: "Christian König" Cc: Pan Xinhui Cc: amd-gfx@lists.freedesktop.org Cc: dri-de...@lists.freedesktop.org --- drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c index 1d955652f3ba..92744d0d2c10 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c @@ -329,8 +329,10 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev, int xcc_id) ring->eop_gpu_addr = kiq->eop_gpu_addr; ring->no_scheduler = true; - snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", -xcc_id, ring->me, ring->pipe, ring->queue); + r = snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", +xcc_id, ring->me, ring->pipe, ring->queue); + if (r >= sizeof(ring->name)) + dev_warn(adev->dev, "kiq ring name truncated\n"); r = amdgpu_ring_init(adev, ring, 1024, irq, AMDGPU_CP_KIQ_IRQ_DRIVER0, AMDGPU_RING_PRIO_DEFAULT, NULL); if (r) -- 2.39.2
[PATCH 3/4] drm/imx: fix -Wformat-truncation warning in imx_ldb_probe()
Enabling -Wformat-truncation yields the following warning:
../drivers/gpu/drm/imx/ipuv3/imx-ldb.c: In function ‘imx_ldb_probe’:
../drivers/gpu/drm/imx/ipuv3/imx-ldb.c:658:57: error: ‘_sel’ directive output
may be truncated writing 4 bytes into a region of size between 3 and 13
[-Werror=format-truncation=]
658 | snprintf(clkname, sizeof(clkname), "di%d_sel", i);
| ^~~~
../drivers/gpu/drm/imx/ipuv3/imx-ldb.c:658:17: note: ‘snprintf’ output between
8 and 18 bytes into a destination of size 16
658 | snprintf(clkname, sizeof(clkname), "di%d_sel", i);
| ^
Silence the warning by checking the snprintf() return value.
Signed-off-by: Jani Nikula
---
Cc: Philipp Zabel
Cc: Shawn Guo
Cc: Sascha Hauer
Cc: Pengutronix Kernel Team
Cc: Fabio Estevam
Cc: dri-de...@lists.freedesktop.org
Cc: i...@lists.linux.dev
---
drivers/gpu/drm/imx/ipuv3/imx-ldb.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
index 71d70194fcbd..46f779fe60ee 100644
--- a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
+++ b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
@@ -654,8 +654,12 @@ static int imx_ldb_probe(struct platform_device *pdev)
*/
for (i = 0; i < 4; i++) {
char clkname[16];
+ int len;
+
+ len = snprintf(clkname, sizeof(clkname), "di%d_sel", i);
+ if (len >= sizeof(clkname))
+ dev_err(dev, "clkname truncated\n");
- snprintf(clkname, sizeof(clkname), "di%d_sel", i);
imx_ldb->clk_sel[i] = devm_clk_get(imx_ldb->dev, clkname);
if (IS_ERR(imx_ldb->clk_sel[i])) {
ret = PTR_ERR(imx_ldb->clk_sel[i]);
--
2.39.2
[PATCH 2/3] drm/xe: drop redundant W=1 warnings from Makefile
Since commit a61ddb4393ad ("drm: enable (most) W=1 warnings by default
across the subsystem"), most of the extra warnings in the driver
Makefile are redundant. Remove them.
Note that -Wmissing-declarations and -Wmissing-prototypes are always
enabled by default in scripts/Makefile.extrawarn.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/xe/Makefile | 25 +
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index c9f067b8f54d..f4366cb958be 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -3,31 +3,8 @@
# Makefile for the drm device driver. This driver provides support for the
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
-# Unconditionally enable W=1 warnings locally
-# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn
-subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter
-subdir-ccflags-y += -Wmissing-declarations
-subdir-ccflags-y += $(call cc-option, -Wrestrict)
-subdir-ccflags-y += -Wmissing-format-attribute
-subdir-ccflags-y += -Wmissing-prototypes
-subdir-ccflags-y += -Wold-style-definition
-subdir-ccflags-y += -Wmissing-include-dirs
-subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable)
-subdir-ccflags-y += $(call cc-option, -Wunused-const-variable)
-subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned)
-subdir-ccflags-y += $(call cc-option, -Wformat-overflow)
+# Enable W=1 warnings not enabled in drm subsystem Makefile
subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
-subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
-# The following turn off the warnings enabled by -Wextra
-ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
-subdir-ccflags-y += -Wno-missing-field-initializers
-subdir-ccflags-y += -Wno-type-limits
-subdir-ccflags-y += -Wno-shift-negative-value
-endif
-ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),)
-subdir-ccflags-y += -Wno-sign-compare
-endif
-# --- end copy-paste
# Enable -Werror in CI and development
subdir-ccflags-$(CONFIG_DRM_XE_WERROR) += -Werror
--
2.39.2
Re: [RESEND 0/6] drm, nouveau/radeon/amdpgu: edid_blob_ptr cleanups
On Mon, 13 May 2024, Alex Deucher wrote: > On Mon, May 13, 2024 at 8:20 AM Jani Nikula wrote: >> >> On Fri, 10 May 2024, Alex Deucher wrote: >> > On Fri, May 10, 2024 at 11:17 AM Jani Nikula wrote: >> > Series is: >> > Acked-by: Alex Deucher >> >> Thanks, do you want to pick these up via your tree? And do you expect a >> proper R-b before merging? > > Feel free to take them via drm-misc if you'd prefer to land the whole > set together, otherwise, I can pick up the radeon/amdgpu patches. Thanks, merged everything to drm-misc-next. BR, Jani. -- Jani Nikula, Intel
Re: [RESEND 0/6] drm, nouveau/radeon/amdpgu: edid_blob_ptr cleanups
On Fri, 10 May 2024, Alex Deucher wrote: > On Fri, May 10, 2024 at 11:17 AM Jani Nikula wrote: >> >> I've sent this some moths ago, let's try again... >> >> BR, >> Jani. >> >> Jani Nikula (6): >> drm/nouveau: convert to using is_hdmi and has_audio from display info >> drm/radeon: convert to using is_hdmi and has_audio from display info >> drm/radeon: remove radeon_connector_edid() and stop using >> edid_blob_ptr >> drm/amdgpu: remove amdgpu_connector_edid() and stop using >> edid_blob_ptr >> drm/edid: add a helper for EDID sysfs property show >> drm/connector: update edid_blob_ptr documentation > > Series is: > Acked-by: Alex Deucher Thanks, do you want to pick these up via your tree? And do you expect a proper R-b before merging? BR, Jani. > >> >> .../gpu/drm/amd/amdgpu/amdgpu_connectors.c| 16 - >> .../gpu/drm/amd/amdgpu/amdgpu_connectors.h| 1 - >> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 4 +-- >> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 4 +-- >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 4 +-- >> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 4 +-- >> drivers/gpu/drm/drm_crtc_internal.h | 2 ++ >> drivers/gpu/drm/drm_edid.c| 33 +++ >> drivers/gpu/drm/drm_sysfs.c | 24 ++ >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 8 ++--- >> drivers/gpu/drm/nouveau/dispnv50/head.c | 8 + >> drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +- >> drivers/gpu/drm/radeon/atombios_encoders.c| 10 +++--- >> drivers/gpu/drm/radeon/evergreen_hdmi.c | 5 ++- >> drivers/gpu/drm/radeon/radeon_audio.c | 13 >> drivers/gpu/drm/radeon/radeon_connectors.c| 27 --- >> drivers/gpu/drm/radeon/radeon_display.c | 2 +- >> drivers/gpu/drm/radeon/radeon_encoders.c | 4 +-- >> drivers/gpu/drm/radeon/radeon_mode.h | 2 -- >> include/drm/drm_connector.h | 6 +++- >> 20 files changed, 79 insertions(+), 100 deletions(-) >> >> -- >> 2.39.2 >> -- Jani Nikula, Intel
Re: [RESEND 1/6] drm/nouveau: convert to using is_hdmi and has_audio from display info
On Fri, 10 May 2024, Lyude Paul wrote:
> Reviewed-by: Lyude Paul
Thanks, how do you want to handle merging this?
BR,
Jani.
>
> On Fri, 2024-05-10 at 18:08 +0300, Jani Nikula wrote:
>> Prefer the parsed results for is_hdmi and has_audio in display info
>> over
>> calling drm_detect_hdmi_monitor() and drm_detect_monitor_audio(),
>> respectively.
>>
>> Conveniently, this also removes the need to use edid_blob_ptr.
>>
>> v2: Reverse a backwards if condition (Ilia)
>>
>> Cc: Karol Herbst
>> Cc: Lyude Paul
>> Cc: Danilo Krummrich
>> Cc: nouv...@lists.freedesktop.org
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/nouveau/dispnv50/disp.c | 8
>> drivers/gpu/drm/nouveau/dispnv50/head.c | 8 +---
>> drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
>> 3 files changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> index 0c3d88ad0b0e..168c27213287 100644
>> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> @@ -751,7 +751,7 @@ nv50_audio_enable(struct drm_encoder *encoder,
>> struct nouveau_crtc *nv_crtc,
>> struct nouveau_encoder *nv_encoder =
>> nouveau_encoder(encoder);
>> struct nvif_outp *outp = &nv_encoder->outp;
>>
>> -if (!nv50_audio_supported(encoder) ||
>> !drm_detect_monitor_audio(nv_connector->edid))
>> +if (!nv50_audio_supported(encoder) || !nv_connector-
>> >base.display_info.has_audio)
>> return;
>>
>> mutex_lock(&drm->audio.lock);
>> @@ -1765,7 +1765,7 @@ nv50_sor_atomic_enable(struct drm_encoder
>> *encoder, struct drm_atomic_state *sta
>> if ((disp->disp->object.oclass == GT214_DISP ||
>> disp->disp->object.oclass >= GF110_DISP) &&
>> nv_encoder->dcb->type != DCB_OUTPUT_LVDS &&
>> - drm_detect_monitor_audio(nv_connector->edid))
>> + nv_connector->base.display_info.has_audio)
>> hda = true;
>>
>> if (!nvif_outp_acquired(outp))
>> @@ -1774,7 +1774,7 @@ nv50_sor_atomic_enable(struct drm_encoder
>> *encoder, struct drm_atomic_state *sta
>> switch (nv_encoder->dcb->type) {
>> case DCB_OUTPUT_TMDS:
>> if (disp->disp->object.oclass != NV50_DISP &&
>> - drm_detect_hdmi_monitor(nv_connector->edid))
>> + nv_connector->base.display_info.is_hdmi)
>> nv50_hdmi_enable(encoder, nv_crtc,
>> nv_connector, state, mode, hda);
>>
>> if (nv_encoder->outp.or.link & 1) {
>> @@ -1787,7 +1787,7 @@ nv50_sor_atomic_enable(struct drm_encoder
>> *encoder, struct drm_atomic_state *sta
>> */
>> if (mode->clock >= 165000 &&
>> nv_encoder->dcb->duallink_possible &&
>> - !drm_detect_hdmi_monitor(nv_connector-
>> >edid))
>> + !nv_connector-
>> >base.display_info.is_hdmi)
>> proto =
>> NV507D_SOR_SET_CONTROL_PROTOCOL_DUAL_TMDS;
>> } else {
>> proto =
>> NV507D_SOR_SET_CONTROL_PROTOCOL_SINGLE_TMDS_B;
>> diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c
>> b/drivers/gpu/drm/nouveau/dispnv50/head.c
>> index 83355dbc15ee..d7c74cc43ba5 100644
>> --- a/drivers/gpu/drm/nouveau/dispnv50/head.c
>> +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
>> @@ -127,14 +127,8 @@ nv50_head_atomic_check_view(struct
>> nv50_head_atom *armh,
>> struct drm_display_mode *omode = &asyh->state.adjusted_mode;
>> struct drm_display_mode *umode = &asyh->state.mode;
>> int mode = asyc->scaler.mode;
>> -struct edid *edid;
>> int umode_vdisplay, omode_hdisplay, omode_vdisplay;
>>
>> -if (connector->edid_blob_ptr)
>> -edid = (struct edid *)connector->edid_blob_ptr-
>> >data;
>> -else
>> -edid = NULL;
>> -
>> if (!asyc->scaler.full) {
>> if (mode == DRM_MODE_SCALE_NONE)
>> omode = umode;
>> @@ -162,7 +156,7 @@ nv50_head_atomic_check_view(struct nv50_head_atom
>> *armh,
>> */
>> if ((asyc->scaler.underscan.mode == UNDERSCAN_ON ||
>> (asyc->sca
[RESEND 2/6] drm/radeon: convert to using is_hdmi and has_audio from display info
Prefer the parsed results for is_hdmi and has_audio in display info over
calling drm_detect_hdmi_monitor() and drm_detect_monitor_audio(),
respectively.
Cc: Alex Deucher
Cc: Christian König
Cc: Pan, Xinhui
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/radeon/atombios_encoders.c | 10 +-
drivers/gpu/drm/radeon/evergreen_hdmi.c| 5 ++---
drivers/gpu/drm/radeon/radeon_audio.c | 6 +++---
drivers/gpu/drm/radeon/radeon_connectors.c | 12 ++--
drivers/gpu/drm/radeon/radeon_display.c| 2 +-
drivers/gpu/drm/radeon/radeon_encoders.c | 4 ++--
6 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c
b/drivers/gpu/drm/radeon/atombios_encoders.c
index 2bff0d9e20f5..0aa395fac36f 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -701,7 +701,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
if (radeon_connector->use_digital &&
(radeon_connector->audio == RADEON_AUDIO_ENABLE))
return ATOM_ENCODER_MODE_HDMI;
- else if
(drm_detect_hdmi_monitor(radeon_connector_edid(connector)) &&
+ else if (connector->display_info.is_hdmi &&
(radeon_connector->audio == RADEON_AUDIO_AUTO))
return ATOM_ENCODER_MODE_HDMI;
else if (radeon_connector->use_digital)
@@ -720,7 +720,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
if (radeon_audio != 0) {
if (radeon_connector->audio == RADEON_AUDIO_ENABLE)
return ATOM_ENCODER_MODE_HDMI;
- else if
(drm_detect_hdmi_monitor(radeon_connector_edid(connector)) &&
+ else if (connector->display_info.is_hdmi &&
(radeon_connector->audio == RADEON_AUDIO_AUTO))
return ATOM_ENCODER_MODE_HDMI;
else
@@ -737,14 +737,14 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
if ((dig_connector->dp_sink_type ==
CONNECTOR_OBJECT_ID_DISPLAYPORT) ||
(dig_connector->dp_sink_type == CONNECTOR_OBJECT_ID_eDP)) {
if (radeon_audio != 0 &&
-
drm_detect_monitor_audio(radeon_connector_edid(connector)) &&
+ connector->display_info.has_audio &&
ASIC_IS_DCE4(rdev) && !ASIC_IS_DCE5(rdev))
return ATOM_ENCODER_MODE_DP_AUDIO;
return ATOM_ENCODER_MODE_DP;
} else if (radeon_audio != 0) {
if (radeon_connector->audio == RADEON_AUDIO_ENABLE)
return ATOM_ENCODER_MODE_HDMI;
- else if
(drm_detect_hdmi_monitor(radeon_connector_edid(connector)) &&
+ else if (connector->display_info.is_hdmi &&
(radeon_connector->audio == RADEON_AUDIO_AUTO))
return ATOM_ENCODER_MODE_HDMI;
else
@@ -755,7 +755,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
break;
case DRM_MODE_CONNECTOR_eDP:
if (radeon_audio != 0 &&
- drm_detect_monitor_audio(radeon_connector_edid(connector))
&&
+ connector->display_info.has_audio &&
ASIC_IS_DCE4(rdev) && !ASIC_IS_DCE5(rdev))
return ATOM_ENCODER_MODE_DP_AUDIO;
return ATOM_ENCODER_MODE_DP;
diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c
b/drivers/gpu/drm/radeon/evergreen_hdmi.c
index 681119c91d94..09dda114e218 100644
--- a/drivers/gpu/drm/radeon/evergreen_hdmi.c
+++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c
@@ -412,7 +412,7 @@ void evergreen_hdmi_enable(struct drm_encoder *encoder,
bool enable)
if (enable) {
struct drm_connector *connector =
radeon_get_connector_for_encoder(encoder);
- if (connector &&
drm_detect_monitor_audio(radeon_connector_edid(connector))) {
+ if (connector && connector->display_info.has_audio) {
WREG32(HDMI_INFOFRAME_CONTROL0 + dig->afmt->offset,
HDMI_AVI_INFO_SEND | /* enable AVI info frames */
HDMI_AVI_INFO_CONT | /* required for audio info
values to be updated */
@@ -450,8 +450,7 @@ void evergreen_dp_enable(struct drm_encoder *encoder, bool
enable)
if (!dig || !dig->afmt)
[RESEND 5/6] drm/edid: add a helper for EDID sysfs property show
Add a helper to get the EDID property for sysfs property show. This
hides all the edid_blob_ptr usage within drm_edid.c.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/drm_crtc_internal.h | 2 ++
drivers/gpu/drm/drm_edid.c | 33 +
drivers/gpu/drm/drm_sysfs.c | 24 ++---
3 files changed, 37 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index 25aaae937ceb..20e9d7b206a2 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -303,6 +303,8 @@ const u8 *drm_edid_find_extension(const struct drm_edid
*drm_edid,
int ext_id, int *ext_index);
void drm_edid_cta_sad_get(const struct cea_sad *cta_sad, u8 *sad);
void drm_edid_cta_sad_set(struct cea_sad *cta_sad, const u8 *sad);
+ssize_t drm_edid_connector_property_show(struct drm_connector *connector,
+char *buf, loff_t off, size_t count);
/* drm_edid_load.c */
#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 4f54c91b31b2..97362dd2330b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -6969,6 +6969,39 @@ static int _drm_edid_connector_property_update(struct
drm_connector *connector,
return ret;
}
+/* For sysfs edid show implementation */
+ssize_t drm_edid_connector_property_show(struct drm_connector *connector,
+char *buf, loff_t off, size_t count)
+{
+ const void *edid;
+ size_t size;
+ ssize_t ret = 0;
+
+ mutex_lock(&connector->dev->mode_config.mutex);
+
+ if (!connector->edid_blob_ptr)
+ goto unlock;
+
+ edid = connector->edid_blob_ptr->data;
+ size = connector->edid_blob_ptr->length;
+ if (!edid)
+ goto unlock;
+
+ if (off >= size)
+ goto unlock;
+
+ if (off + count > size)
+ count = size - off;
+
+ memcpy(buf, edid + off, count);
+
+ ret = count;
+unlock:
+ mutex_unlock(&connector->dev->mode_config.mutex);
+
+ return ret;
+}
+
/**
* drm_edid_connector_update - Update connector information from EDID
* @connector: Connector
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index bd9b8ab4f82b..fb3bbb6adcd1 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -266,29 +266,9 @@ static ssize_t edid_show(struct file *filp, struct kobject
*kobj,
{
struct device *connector_dev = kobj_to_dev(kobj);
struct drm_connector *connector = to_drm_connector(connector_dev);
- unsigned char *edid;
- size_t size;
- ssize_t ret = 0;
+ ssize_t ret;
- mutex_lock(&connector->dev->mode_config.mutex);
- if (!connector->edid_blob_ptr)
- goto unlock;
-
- edid = connector->edid_blob_ptr->data;
- size = connector->edid_blob_ptr->length;
- if (!edid)
- goto unlock;
-
- if (off >= size)
- goto unlock;
-
- if (off + count > size)
- count = size - off;
- memcpy(buf, edid + off, count);
-
- ret = count;
-unlock:
- mutex_unlock(&connector->dev->mode_config.mutex);
+ ret = drm_edid_connector_property_show(connector, buf, off, count);
return ret;
}
--
2.39.2
[RESEND 0/6] drm, nouveau/radeon/amdpgu: edid_blob_ptr cleanups
I've sent this some moths ago, let's try again... BR, Jani. Jani Nikula (6): drm/nouveau: convert to using is_hdmi and has_audio from display info drm/radeon: convert to using is_hdmi and has_audio from display info drm/radeon: remove radeon_connector_edid() and stop using edid_blob_ptr drm/amdgpu: remove amdgpu_connector_edid() and stop using edid_blob_ptr drm/edid: add a helper for EDID sysfs property show drm/connector: update edid_blob_ptr documentation .../gpu/drm/amd/amdgpu/amdgpu_connectors.c| 16 - .../gpu/drm/amd/amdgpu/amdgpu_connectors.h| 1 - drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 4 +-- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 4 +-- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 4 +-- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 4 +-- drivers/gpu/drm/drm_crtc_internal.h | 2 ++ drivers/gpu/drm/drm_edid.c| 33 +++ drivers/gpu/drm/drm_sysfs.c | 24 ++ drivers/gpu/drm/nouveau/dispnv50/disp.c | 8 ++--- drivers/gpu/drm/nouveau/dispnv50/head.c | 8 + drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +- drivers/gpu/drm/radeon/atombios_encoders.c| 10 +++--- drivers/gpu/drm/radeon/evergreen_hdmi.c | 5 ++- drivers/gpu/drm/radeon/radeon_audio.c | 13 drivers/gpu/drm/radeon/radeon_connectors.c| 27 --- drivers/gpu/drm/radeon/radeon_display.c | 2 +- drivers/gpu/drm/radeon/radeon_encoders.c | 4 +-- drivers/gpu/drm/radeon/radeon_mode.h | 2 -- include/drm/drm_connector.h | 6 +++- 20 files changed, 79 insertions(+), 100 deletions(-) -- 2.39.2
[RESEND 6/6] drm/connector: update edid_blob_ptr documentation
Accessing the EDID via edid_blob_ptr causes chicken-and-egg
problems. Keep edid_blob_ptr as the userspace interface that should be
accessed via dedicated functions.
Signed-off-by: Jani Nikula
---
include/drm/drm_connector.h | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index fe88d7fc6b8f..58ee9adf9091 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1636,8 +1636,12 @@ struct drm_connector {
/**
* @edid_blob_ptr: DRM property containing EDID if present. Protected by
-* &drm_mode_config.mutex. This should be updated only by calling
+* &drm_mode_config.mutex.
+*
+* This must be updated only by calling drm_edid_connector_update() or
* drm_connector_update_edid_property().
+*
+* This must not be used by drivers directly.
*/
struct drm_property_blob *edid_blob_ptr;
--
2.39.2
[RESEND 1/6] drm/nouveau: convert to using is_hdmi and has_audio from display info
Prefer the parsed results for is_hdmi and has_audio in display info over
calling drm_detect_hdmi_monitor() and drm_detect_monitor_audio(),
respectively.
Conveniently, this also removes the need to use edid_blob_ptr.
v2: Reverse a backwards if condition (Ilia)
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: nouv...@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/nouveau/dispnv50/disp.c | 8
drivers/gpu/drm/nouveau/dispnv50/head.c | 8 +---
drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +-
3 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 0c3d88ad0b0e..168c27213287 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -751,7 +751,7 @@ nv50_audio_enable(struct drm_encoder *encoder, struct
nouveau_crtc *nv_crtc,
struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder);
struct nvif_outp *outp = &nv_encoder->outp;
- if (!nv50_audio_supported(encoder) ||
!drm_detect_monitor_audio(nv_connector->edid))
+ if (!nv50_audio_supported(encoder) ||
!nv_connector->base.display_info.has_audio)
return;
mutex_lock(&drm->audio.lock);
@@ -1765,7 +1765,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder,
struct drm_atomic_state *sta
if ((disp->disp->object.oclass == GT214_DISP ||
disp->disp->object.oclass >= GF110_DISP) &&
nv_encoder->dcb->type != DCB_OUTPUT_LVDS &&
- drm_detect_monitor_audio(nv_connector->edid))
+ nv_connector->base.display_info.has_audio)
hda = true;
if (!nvif_outp_acquired(outp))
@@ -1774,7 +1774,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder,
struct drm_atomic_state *sta
switch (nv_encoder->dcb->type) {
case DCB_OUTPUT_TMDS:
if (disp->disp->object.oclass != NV50_DISP &&
- drm_detect_hdmi_monitor(nv_connector->edid))
+ nv_connector->base.display_info.is_hdmi)
nv50_hdmi_enable(encoder, nv_crtc, nv_connector, state,
mode, hda);
if (nv_encoder->outp.or.link & 1) {
@@ -1787,7 +1787,7 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder,
struct drm_atomic_state *sta
*/
if (mode->clock >= 165000 &&
nv_encoder->dcb->duallink_possible &&
- !drm_detect_hdmi_monitor(nv_connector->edid))
+ !nv_connector->base.display_info.is_hdmi)
proto =
NV507D_SOR_SET_CONTROL_PROTOCOL_DUAL_TMDS;
} else {
proto = NV507D_SOR_SET_CONTROL_PROTOCOL_SINGLE_TMDS_B;
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c
b/drivers/gpu/drm/nouveau/dispnv50/head.c
index 83355dbc15ee..d7c74cc43ba5 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -127,14 +127,8 @@ nv50_head_atomic_check_view(struct nv50_head_atom *armh,
struct drm_display_mode *omode = &asyh->state.adjusted_mode;
struct drm_display_mode *umode = &asyh->state.mode;
int mode = asyc->scaler.mode;
- struct edid *edid;
int umode_vdisplay, omode_hdisplay, omode_vdisplay;
- if (connector->edid_blob_ptr)
- edid = (struct edid *)connector->edid_blob_ptr->data;
- else
- edid = NULL;
-
if (!asyc->scaler.full) {
if (mode == DRM_MODE_SCALE_NONE)
omode = umode;
@@ -162,7 +156,7 @@ nv50_head_atomic_check_view(struct nv50_head_atom *armh,
*/
if ((asyc->scaler.underscan.mode == UNDERSCAN_ON ||
(asyc->scaler.underscan.mode == UNDERSCAN_AUTO &&
-drm_detect_hdmi_monitor(edid {
+connector->display_info.is_hdmi))) {
u32 bX = asyc->scaler.underscan.hborder;
u32 bY = asyc->scaler.underscan.vborder;
u32 r = (asyh->view.oH << 19) / asyh->view.oW;
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 856b3ef5edb8..938832a6af15 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1034,7 +1034,7 @@ get_tmds_link_bandwidth(struct drm_connector *connector)
unsigned duallink_scale =
nouveau_duallink && nv_encoder->dcb->duallink_possible ? 2 : 1;
- if (drm_detect_hdmi_monitor(nv_connector->edid)) {
+ if (nv_connector->base.display_info.is_hdmi) {
info = &nv_connector->base.display_info;
duallink_scale = 1;
}
--
2.39.2
[RESEND 3/6] drm/radeon: remove radeon_connector_edid() and stop using edid_blob_ptr
radeon_connector_edid() copies the EDID from edid_blob_ptr as a side
effect if radeon_connector->edid isn't initialized. However, everywhere
that the returned EDID is used, the EDID should have been set
beforehands.
Only the drm EDID code should look at the EDID property, anyway, so stop
using it.
Cc: Alex Deucher
Cc: Christian König
Cc: Pan, Xinhui
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/radeon/radeon_audio.c | 7 ---
drivers/gpu/drm/radeon/radeon_connectors.c | 15 ---
drivers/gpu/drm/radeon/radeon_mode.h | 2 --
3 files changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
b/drivers/gpu/drm/radeon/radeon_audio.c
index 16c10db3ce6f..0bcd767b9f47 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -303,6 +303,7 @@ void radeon_audio_endpoint_wreg(struct radeon_device *rdev,
u32 offset,
static void radeon_audio_write_sad_regs(struct drm_encoder *encoder)
{
struct drm_connector *connector =
radeon_get_connector_for_encoder(encoder);
+ struct radeon_connector *radeon_connector =
to_radeon_connector(connector);
struct radeon_encoder *radeon_encoder = to_radeon_encoder(encoder);
struct cea_sad *sads;
int sad_count;
@@ -310,7 +311,7 @@ static void radeon_audio_write_sad_regs(struct drm_encoder
*encoder)
if (!connector)
return;
- sad_count = drm_edid_to_sad(radeon_connector_edid(connector), &sads);
+ sad_count = drm_edid_to_sad(radeon_connector->edid, &sads);
if (sad_count < 0)
DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
if (sad_count <= 0)
@@ -326,6 +327,7 @@ static void radeon_audio_write_sad_regs(struct drm_encoder
*encoder)
static void radeon_audio_write_speaker_allocation(struct drm_encoder *encoder)
{
struct drm_connector *connector =
radeon_get_connector_for_encoder(encoder);
+ struct radeon_connector *radeon_connector =
to_radeon_connector(connector);
struct radeon_encoder *radeon_encoder = to_radeon_encoder(encoder);
u8 *sadb = NULL;
int sad_count;
@@ -333,8 +335,7 @@ static void radeon_audio_write_speaker_allocation(struct
drm_encoder *encoder)
if (!connector)
return;
- sad_count =
drm_edid_to_speaker_allocation(radeon_connector_edid(connector),
- &sadb);
+ sad_count = drm_edid_to_speaker_allocation(radeon_connector->edid,
&sadb);
if (sad_count < 0) {
DRM_DEBUG("Couldn't read Speaker Allocation Data Block: %d\n",
sad_count);
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
b/drivers/gpu/drm/radeon/radeon_connectors.c
index 81b5c3c8f658..80879e946342 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -255,21 +255,6 @@ static struct drm_encoder *radeon_find_encoder(struct
drm_connector *connector,
return NULL;
}
-struct edid *radeon_connector_edid(struct drm_connector *connector)
-{
- struct radeon_connector *radeon_connector =
to_radeon_connector(connector);
- struct drm_property_blob *edid_blob = connector->edid_blob_ptr;
-
- if (radeon_connector->edid) {
- return radeon_connector->edid;
- } else if (edid_blob) {
- struct edid *edid = kmemdup(edid_blob->data, edid_blob->length,
GFP_KERNEL);
- if (edid)
- radeon_connector->edid = edid;
- }
- return radeon_connector->edid;
-}
-
static void radeon_connector_get_edid(struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h
b/drivers/gpu/drm/radeon/radeon_mode.h
index 546381a5c918..e0a5af180801 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -701,8 +701,6 @@ extern u16
radeon_connector_encoder_get_dp_bridge_encoder_id(struct drm_connecto
extern bool radeon_connector_is_dp12_capable(struct drm_connector *connector);
extern int radeon_get_monitor_bpc(struct drm_connector *connector);
-extern struct edid *radeon_connector_edid(struct drm_connector *connector);
-
extern void radeon_connector_hotplug(struct drm_connector *connector);
extern int radeon_dp_mode_valid_helper(struct drm_connector *connector,
struct drm_display_mode *mode);
--
2.39.2
[RESEND 4/6] drm/amdgpu: remove amdgpu_connector_edid() and stop using edid_blob_ptr
amdgpu_connector_edid() copies the EDID from edid_blob_ptr as a side
effect if amdgpu_connector->edid isn't initialized. However, everywhere
that the returned EDID is used, the EDID should have been set
beforehands.
Only the drm EDID code should look at the EDID property, anyway, so stop
using it.
Cc: Alex Deucher
Cc: Christian König
Cc: Pan, Xinhui
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 16
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h | 1 -
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 4 ++--
6 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 9caba10315a8..cae7479c3ecf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -246,22 +246,6 @@ amdgpu_connector_find_encoder(struct drm_connector
*connector,
return NULL;
}
-struct edid *amdgpu_connector_edid(struct drm_connector *connector)
-{
- struct amdgpu_connector *amdgpu_connector =
to_amdgpu_connector(connector);
- struct drm_property_blob *edid_blob = connector->edid_blob_ptr;
-
- if (amdgpu_connector->edid) {
- return amdgpu_connector->edid;
- } else if (edid_blob) {
- struct edid *edid = kmemdup(edid_blob->data, edid_blob->length,
GFP_KERNEL);
-
- if (edid)
- amdgpu_connector->edid = edid;
- }
- return amdgpu_connector->edid;
-}
-
static struct edid *
amdgpu_connector_get_hardcoded_edid(struct amdgpu_device *adev)
{
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
index 61fcef15ad72..eff833b6ed31 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
@@ -24,7 +24,6 @@
#ifndef __AMDGPU_CONNECTORS_H__
#define __AMDGPU_CONNECTORS_H__
-struct edid *amdgpu_connector_edid(struct drm_connector *connector);
void amdgpu_connector_hotplug(struct drm_connector *connector);
int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector);
u16 amdgpu_connector_encoder_get_dp_bridge_encoder_id(struct drm_connector
*connector);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index b44fce44c066..dddb5fe16f2c 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1299,7 +1299,7 @@ static void
dce_v10_0_audio_write_speaker_allocation(struct drm_encoder *encoder
return;
}
- sad_count =
drm_edid_to_speaker_allocation(amdgpu_connector_edid(connector), &sadb);
+ sad_count = drm_edid_to_speaker_allocation(amdgpu_connector->edid,
&sadb);
if (sad_count < 0) {
DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n",
sad_count);
sad_count = 0;
@@ -1369,7 +1369,7 @@ static void dce_v10_0_audio_write_sad_regs(struct
drm_encoder *encoder)
return;
}
- sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
+ sad_count = drm_edid_to_sad(amdgpu_connector->edid, &sads);
if (sad_count < 0)
DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
if (sad_count <= 0)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 80b2e7f79acf..11780e4d7e9f 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1331,7 +1331,7 @@ static void
dce_v11_0_audio_write_speaker_allocation(struct drm_encoder *encoder
return;
}
- sad_count =
drm_edid_to_speaker_allocation(amdgpu_connector_edid(connector), &sadb);
+ sad_count = drm_edid_to_speaker_allocation(amdgpu_connector->edid,
&sadb);
if (sad_count < 0) {
DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n",
sad_count);
sad_count = 0;
@@ -1401,7 +1401,7 @@ static void dce_v11_0_audio_write_sad_regs(struct
drm_encoder *encoder)
return;
}
- sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
+ sad_count = drm_edid_to_sad(amdgpu_connector->edid, &sads);
if (sad_count < 0)
DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
if (sad_count <= 0)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index db20012600f5..05c0df97f01d 100644
--- a/drivers/gpu/
RE: [PATCH] drm/mst: Fix NULL pointer dereference at drm_dp_add_payload_part2
On Fri, 10 May 2024, "Lin, Wayne" wrote:
> [Public]
>
>> -Original Message-
>> From: Limonciello, Mario
>> Sent: Friday, May 10, 2024 3:18 AM
>> To: Linux regressions mailing list ; Wentland,
>> Harry
>> ; Lin, Wayne
>> Cc: ly...@redhat.com; imre.d...@intel.com; Leon Weiß > bochum.de>; sta...@vger.kernel.org; dri-de...@lists.freedesktop.org; amd-
>> g...@lists.freedesktop.org; intel-...@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/mst: Fix NULL pointer dereference at
>> drm_dp_add_payload_part2
>>
>> On 5/9/2024 07:43, Linux regression tracking (Thorsten Leemhuis) wrote:
>> > On 18.04.24 21:43, Harry Wentland wrote:
>> >> On 2024-03-07 01:29, Wayne Lin wrote:
>> >>> [Why]
>> >>> Commit:
>> >>> - commit 5aa1dfcdf0a4 ("drm/mst: Refactor the flow for payload
>> >>> allocation/removement") accidently overwrite the commit
>> >>> - commit 54d217406afe ("drm: use mgr->dev in drm_dbg_kms in
>> >>> drm_dp_add_payload_part2") which cause regression.
>> >>>
>> >>> [How]
>> >>> Recover the original NULL fix and remove the unnecessary input
>> >>> parameter 'state' for drm_dp_add_payload_part2().
>> >>>
>> >>> Fixes: 5aa1dfcdf0a4 ("drm/mst: Refactor the flow for payload
>> >>> allocation/removement")
>> >>> Reported-by: Leon Weiß
>> >>> Link:
>> >>> https://lore.kernel.org/r/38c253ea42072cc825dc969ac4e6b9b600371cc8.c
>> >>> a...@ruhr-uni-bochum.de/
>> >>> Cc: ly...@redhat.com
>> >>> Cc: imre.d...@intel.com
>> >>> Cc: sta...@vger.kernel.org
>> >>> Cc: regressi...@lists.linux.dev
>> >>> Signed-off-by: Wayne Lin
>> >>
>> >> I haven't been deep in MST code in a while but this all looks pretty
>> >> straightforward and good.
>> >>
>> >> Reviewed-by: Harry Wentland
>> >
>> > Hmmm, that was three weeks ago, but it seems since then nothing
>> > happened to fix the linked regression through this or some other
>> > patch. Is there a reason? The build failure report from the CI maybe?
>>
>> It touches files outside of amd but only has an ack from AMD. I think we
>> /probably/ want an ack from i915 and nouveau to take it through.
>
> Thanks, Mario!
>
> Hi Thorsten,
> Yeah, like what Mario said. Would also like to have ack from i915 and nouveau.
It usually works better if you Cc the folks you want an ack from! ;)
Acked-by: Jani Nikula
>
>>
>> >
>> > Wayne Lin, do you know what's up?
>> >
>> > Ciao, Thorsten
>> >
>> >>> ---
>> >>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
>> >>> drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +---
>> >>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +-
>> >>> drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
>> >>> include/drm/display/drm_dp_mst_helper.h | 1 -
>> >>> 5 files changed, 4 insertions(+), 7 deletions(-)
>> >>>
>> >>> diff --git
>> >>> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> >>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> >>> index c27063305a13..2c36f3d00ca2 100644
>> >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> >>> @@ -363,7 +363,7 @@ void dm_helpers_dp_mst_send_payload_allocation(
>> >>> mst_state = to_drm_dp_mst_topology_state(mst_mgr->base.state);
>> >>> new_payload = drm_atomic_get_mst_payload_state(mst_state,
>> >>> aconnector->mst_output_port);
>> >>>
>> >>> - ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
>> new_payload);
>> >>> + ret = drm_dp_add_payload_part2(mst_mgr, new_payload);
>> >>>
>> >>> if (ret) {
>> >>> amdgpu_dm_set_mst_status(&aconnector->mst_status,
>> >>> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> >>> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> >>> index 03d528209426..95fd18f24e94 100644
>> >>> --- a/drivers/gpu/drm/d
Re: [PATCH 2/5] drm/amdgpu: Use drm_crtc_vblank_crtc()
On Mon, 08 Apr 2024, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Replace the open coded drm_crtc_vblank_crtc() with the real
> thing.
>
> Cc: Alex Deucher
> Cc: "Christian König"
> Cc: "Pan, Xinhui"
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä
FWIW,
Reviewed-by: Jani Nikula
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 8 ++--
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> 2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> index 8baa2e0935cc..258703145161 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
> @@ -65,9 +65,7 @@ static enum hrtimer_restart
> amdgpu_vkms_vblank_simulate(struct hrtimer *timer)
>
> static int amdgpu_vkms_enable_vblank(struct drm_crtc *crtc)
> {
> - struct drm_device *dev = crtc->dev;
> - unsigned int pipe = drm_crtc_index(crtc);
> - struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> + struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> struct amdgpu_vkms_output *out = drm_crtc_to_amdgpu_vkms_output(crtc);
> struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>
> @@ -91,10 +89,8 @@ static bool amdgpu_vkms_get_vblank_timestamp(struct
> drm_crtc *crtc,
>ktime_t *vblank_time,
>bool in_vblank_irq)
> {
> - struct drm_device *dev = crtc->dev;
> - unsigned int pipe = crtc->index;
> struct amdgpu_vkms_output *output =
> drm_crtc_to_amdgpu_vkms_output(crtc);
> - struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> + struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>
> if (!READ_ONCE(vblank->enabled)) {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 71d2d44681b2..662d2d83473b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -528,7 +528,7 @@ static void dm_vupdate_high_irq(void *interrupt_params)
> if (acrtc) {
> vrr_active = amdgpu_dm_crtc_vrr_active_irq(acrtc);
> drm_dev = acrtc->base.dev;
> - vblank = &drm_dev->vblank[acrtc->base.index];
> + vblank = drm_crtc_vblank_crtc(&acrtc->base);
> previous_timestamp =
> atomic64_read(&irq_params->previous_timestamp);
> frame_duration_ns = vblank->time - previous_timestamp;
--
Jani Nikula, Intel
Re: [PATCH v2 03/12] drm/i915: Make I2C terminology more inclusive
On Fri, 03 May 2024, Rodrigo Vivi wrote: > On Fri, May 03, 2024 at 02:04:15PM -0700, Easwar Hariharan wrote: >> On 5/3/2024 12:34 PM, Rodrigo Vivi wrote: >> > On Fri, May 03, 2024 at 06:13:24PM +, Easwar Hariharan wrote: >> >> I2C v7, SMBus 3.2, and I3C 1.1.1 specifications have replaced >> >> "master/slave" >> >> with more appropriate terms. Inspired by and following on to Wolfram's >> >> series to fix drivers/i2c/[1], fix the terminology for users of >> >> I2C_ALGOBIT bitbanging interface, now that the approved verbiage exists >> >> in the specification. >> >> >> >> Compile tested, no functionality changes intended >> >> >> >> [1]: >> >> https://lore.kernel.org/all/20240322132619.6389-1-wsa+rene...@sang-engineering.com/ >> >> >> >> Reviewed-by: Rodrigo Vivi >> >> Acked-by: Rodrigo Vivi >> > >> > It looks like the ack is not needed since we are merging this through >> > drm-intel-next. But I'm planing to merge this only after seeing the >> > main drivers/i2c accepting the new terminology. So we don't have a >> > risk of that getting push back and new names there and we having >> > to rename it once again. >> >> Just to be explicit, did you want me to remove the Acked-by in v3, or will >> you when you pull >> the patch into drm-intel-next? >> >> > >> > (more below) >> > >> >> Acked-by: Zhi Wang >> >> Signed-off-by: Easwar Hariharan >> > >> > Cc: Jani Nikula >> > >> > Jani, what bits were you concerned that were not necessarily i2c? >> > I believe although not necessarily/directly i2c, I believe they >> > are related and could benefit from the massive single shot renable. >> > or do you have any better split to suggest here? >> > >> > (more below) >> > >> >> --- >> >> drivers/gpu/drm/i915/display/dvo_ch7017.c | 14 - >> >> drivers/gpu/drm/i915/display/dvo_ch7xxx.c | 18 +-- >> >> drivers/gpu/drm/i915/display/dvo_ivch.c | 16 +- >> >> drivers/gpu/drm/i915/display/dvo_ns2501.c | 18 +-- >> >> drivers/gpu/drm/i915/display/dvo_sil164.c | 18 +-- >> >> drivers/gpu/drm/i915/display/dvo_tfp410.c | 18 +-- >> >> drivers/gpu/drm/i915/display/intel_bios.c | 22 +++--- >> >> drivers/gpu/drm/i915/display/intel_ddi.c | 2 +- >> >> .../gpu/drm/i915/display/intel_display_core.h | 2 +- >> >> drivers/gpu/drm/i915/display/intel_dsi.h | 2 +- >> >> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 20 ++--- >> >> drivers/gpu/drm/i915/display/intel_dvo.c | 14 - >> >> drivers/gpu/drm/i915/display/intel_dvo_dev.h | 2 +- >> >> drivers/gpu/drm/i915/display/intel_gmbus.c| 4 +-- >> >> drivers/gpu/drm/i915/display/intel_sdvo.c | 30 +-- >> >> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 4 +-- >> >> drivers/gpu/drm/i915/gvt/edid.c | 28 - >> >> drivers/gpu/drm/i915/gvt/edid.h | 4 +-- >> >> drivers/gpu/drm/i915/gvt/opregion.c | 2 +- >> >> 19 files changed, 119 insertions(+), 119 deletions(-) >> >> >> >> >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c >> >> b/drivers/gpu/drm/i915/display/intel_ddi.c >> >> index c17462b4c2ac..64db211148a8 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >> >> @@ -4332,7 +4332,7 @@ static int intel_ddi_compute_config_late(struct >> >> intel_encoder *encoder, >> >> >> >> connector->tile_group->id); >> >> >> >> /* >> >> - * EDP Transcoders cannot be ensalved >> >> + * EDP Transcoders cannot be slaves >> > >> > ^ here >> > perhaps you meant 'targeted' ? >> > >> >>* make them a master always when present >> >> >> >> This is not actually I2C related as far as I could tell when I was making >> the change, so this was more of a typo fix. >> >> If we want to improve this, a quick check with the eDP v1.5a spec suggests >> using primary/secondary instead, >> though in a global fashion rather than specifically for eDP transcoders. >> There is also source/sink terminology >> in the spec related to DP encoders. >> >> Which would be a more acceptable change here? > > hmmm probably better to split the patches and align with the spec naming > where it applies. > and with i2c name where it applies. Yeah this one is completely unrelated to i2c and aux, and what the eDP spec says is irrelevant here. This should follow Intel hw specs. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 1/2] drm/print: drop include debugfs.h and include where needed
On Mon, 22 Apr 2024, Jani Nikula wrote: > Surprisingly many places depend on debugfs.h to be included via > drm_print.h. Fix them. > > v3: Also fix armada, ite-it6505, imagination, msm, sti, vc4, and xe > > v2: Also fix ivpu and vmwgfx > > Reviewed-by: Andrzej Hajda > Acked-by: Maxime Ripard > Link: > https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com > Signed-off-by: Jani Nikula While the changes all over the place are small, mostly just adding the debugfs.h include, please consider acking. I've sent this a few times already. Otherwise, I'll merge this by the end of the week, acks or not. Thanks, Jani. > > --- > > Cc: Jacek Lawrynowicz > Cc: Stanislaw Gruszka > Cc: Oded Gabbay > Cc: Russell King > Cc: David Airlie > Cc: Daniel Vetter > Cc: Andrzej Hajda > Cc: Neil Armstrong > Cc: Robert Foss > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Jernej Skrabec > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: Jani Nikula > Cc: Rodrigo Vivi > Cc: Joonas Lahtinen > Cc: Tvrtko Ursulin > Cc: Frank Binns > Cc: Matt Coster > Cc: Rob Clark > Cc: Abhinav Kumar > Cc: Dmitry Baryshkov > Cc: Sean Paul > Cc: Marijn Suijten > Cc: Karol Herbst > Cc: Lyude Paul > Cc: Danilo Krummrich > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: Alain Volmat > Cc: Huang Rui > Cc: Zack Rusin > Cc: Broadcom internal kernel review list > > Cc: Lucas De Marchi > Cc: "Thomas Hellström" > Cc: dri-de...@lists.freedesktop.org > Cc: intel-...@lists.freedesktop.org > Cc: intel...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > --- > drivers/accel/ivpu/ivpu_debugfs.c | 2 ++ > drivers/gpu/drm/armada/armada_debugfs.c | 1 + > drivers/gpu/drm/bridge/ite-it6505.c | 1 + > drivers/gpu/drm/bridge/panel.c | 2 ++ > drivers/gpu/drm/drm_print.c | 6 +++--- > drivers/gpu/drm/i915/display/intel_dmc.c| 1 + > drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 ++ > drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ > drivers/gpu/drm/radeon/r100.c | 1 + > drivers/gpu/drm/radeon/r300.c | 1 + > drivers/gpu/drm/radeon/r420.c | 1 + > drivers/gpu/drm/radeon/r600.c | 3 ++- > drivers/gpu/drm/radeon/radeon_fence.c | 1 + > drivers/gpu/drm/radeon/radeon_gem.c | 1 + > drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ > drivers/gpu/drm/radeon/radeon_pm.c | 1 + > drivers/gpu/drm/radeon/radeon_ring.c| 2 ++ > drivers/gpu/drm/radeon/radeon_ttm.c | 1 + > drivers/gpu/drm/radeon/rs400.c | 1 + > drivers/gpu/drm/radeon/rv515.c | 1 + > drivers/gpu/drm/sti/sti_drv.c | 1 + > drivers/gpu/drm/ttm/ttm_device.c| 1 + > drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- > drivers/gpu/drm/ttm/ttm_tt.c| 5 +++-- > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ > drivers/gpu/drm/xe/xe_debugfs.c | 1 + > drivers/gpu/drm/xe/xe_gt_debugfs.c | 2 ++ > drivers/gpu/drm/xe/xe_uc_debugfs.c | 2 ++ > include/drm/drm_print.h | 2 +- > 31 files changed, 46 insertions(+), 8 deletions(-) > > diff --git a/drivers/accel/ivpu/ivpu_debugfs.c > b/drivers/accel/ivpu/ivpu_debugfs.c > index d09d29775b3f..e07e447d08d1 100644 > --- a/drivers/accel/ivpu/ivpu_debugfs.c > +++ b/drivers/accel/ivpu/ivpu_debugfs.c > @@ -3,6 +3,8 @@ > * Copyright (C) 2020-2023 Intel Corporation > */ > > +#include > + > #include > #include > #include > diff --git a/drivers/gpu/drm/armada/armada_debugfs.c > b/drivers/gpu/drm/armada/armada_debugfs.c > index 29f4b52e3c8d..a763349dd89f 100644 > --- a/drivers/gpu/drm/armada/armada_debugfs.c > +++ b/drivers/gpu/drm/armada/armada_debugfs.c > @@ -5,6 +5,7 @@ > */ > > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > index 27334173e911..3f68c82888c2 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2020, The Linux Foundation. All rights reserved. > */ > #include > +#include > #include > #include > #incl
[PATCH 1/2] drm/print: drop include debugfs.h and include where needed
Surprisingly many places depend on debugfs.h to be included via drm_print.h. Fix them. v3: Also fix armada, ite-it6505, imagination, msm, sti, vc4, and xe v2: Also fix ivpu and vmwgfx Reviewed-by: Andrzej Hajda Acked-by: Maxime Ripard Link: https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com Signed-off-by: Jani Nikula --- Cc: Jacek Lawrynowicz Cc: Stanislaw Gruszka Cc: Oded Gabbay Cc: Russell King Cc: David Airlie Cc: Daniel Vetter Cc: Andrzej Hajda Cc: Neil Armstrong Cc: Robert Foss Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: Jani Nikula Cc: Rodrigo Vivi Cc: Joonas Lahtinen Cc: Tvrtko Ursulin Cc: Frank Binns Cc: Matt Coster Cc: Rob Clark Cc: Abhinav Kumar Cc: Dmitry Baryshkov Cc: Sean Paul Cc: Marijn Suijten Cc: Karol Herbst Cc: Lyude Paul Cc: Danilo Krummrich Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: Alain Volmat Cc: Huang Rui Cc: Zack Rusin Cc: Broadcom internal kernel review list Cc: Lucas De Marchi Cc: "Thomas Hellström" Cc: dri-de...@lists.freedesktop.org Cc: intel-...@lists.freedesktop.org Cc: intel...@lists.freedesktop.org Cc: linux-arm-...@vger.kernel.org Cc: freedr...@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Cc: amd-gfx@lists.freedesktop.org --- drivers/accel/ivpu/ivpu_debugfs.c | 2 ++ drivers/gpu/drm/armada/armada_debugfs.c | 1 + drivers/gpu/drm/bridge/ite-it6505.c | 1 + drivers/gpu/drm/bridge/panel.c | 2 ++ drivers/gpu/drm/drm_print.c | 6 +++--- drivers/gpu/drm/i915/display/intel_dmc.c| 1 + drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 ++ drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ drivers/gpu/drm/radeon/r100.c | 1 + drivers/gpu/drm/radeon/r300.c | 1 + drivers/gpu/drm/radeon/r420.c | 1 + drivers/gpu/drm/radeon/r600.c | 3 ++- drivers/gpu/drm/radeon/radeon_fence.c | 1 + drivers/gpu/drm/radeon/radeon_gem.c | 1 + drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ drivers/gpu/drm/radeon/radeon_pm.c | 1 + drivers/gpu/drm/radeon/radeon_ring.c| 2 ++ drivers/gpu/drm/radeon/radeon_ttm.c | 1 + drivers/gpu/drm/radeon/rs400.c | 1 + drivers/gpu/drm/radeon/rv515.c | 1 + drivers/gpu/drm/sti/sti_drv.c | 1 + drivers/gpu/drm/ttm/ttm_device.c| 1 + drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- drivers/gpu/drm/ttm/ttm_tt.c| 5 +++-- drivers/gpu/drm/vc4/vc4_drv.h | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ drivers/gpu/drm/xe/xe_debugfs.c | 1 + drivers/gpu/drm/xe/xe_gt_debugfs.c | 2 ++ drivers/gpu/drm/xe/xe_uc_debugfs.c | 2 ++ include/drm/drm_print.h | 2 +- 31 files changed, 46 insertions(+), 8 deletions(-) diff --git a/drivers/accel/ivpu/ivpu_debugfs.c b/drivers/accel/ivpu/ivpu_debugfs.c index d09d29775b3f..e07e447d08d1 100644 --- a/drivers/accel/ivpu/ivpu_debugfs.c +++ b/drivers/accel/ivpu/ivpu_debugfs.c @@ -3,6 +3,8 @@ * Copyright (C) 2020-2023 Intel Corporation */ +#include + #include #include #include diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c index 29f4b52e3c8d..a763349dd89f 100644 --- a/drivers/gpu/drm/armada/armada_debugfs.c +++ b/drivers/gpu/drm/armada/armada_debugfs.c @@ -5,6 +5,7 @@ */ #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 27334173e911..3f68c82888c2 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -3,6 +3,7 @@ * Copyright (c) 2020, The Linux Foundation. All rights reserved. */ #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 7f41525f7a6e..32506524d9a2 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -4,6 +4,8 @@ * Copyright (C) 2017 Broadcom */ +#include + #include #include #include diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 699b7dbffd7b..cf2efb44722c 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -23,13 +23,13 @@ * Rob Clark */ -#include - +#include +#include #include #include #include #include -#include +#include #include #include diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c index 3697a02b51b6..09346afd1f0e 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -22,6 +22,7 @@ * */ +#include #include #include "i915_drv.h"
Re: [PATCH v2 1/2] drm/print: drop include debugfs.h and include where needed
On Thu, 18 Apr 2024, Jani Nikula wrote: > On Thu, 18 Apr 2024, Robert Foss wrote: >> I'm seeing build errors for drivers/gpu/drm/bridge/ite-it6505.c, is >> this expected? > > No, but it's possible my configs didn't catch all configs. :( Okay, enabled a bunch more arm/arm64 stuff, and hit some more issues. v3 at [1]. BR, Jani. [1] https://lore.kernel.org/r/20240422121011.4133236-1-jani.nik...@intel.com -- Jani Nikula, Intel
Re: [PATCH v2 1/2] drm/print: drop include debugfs.h and include where needed
On Thu, 18 Apr 2024, Robert Foss wrote: > I'm seeing build errors for drivers/gpu/drm/bridge/ite-it6505.c, is > this expected? No, but it's possible my configs didn't catch all configs. :( BR, Jani. -- Jani Nikula, Intel
[PATCH v2 1/2] drm/print: drop include debugfs.h and include where needed
Surprisingly many places depend on debugfs.h to be included via drm_print.h. Fix them. v2: Also fix ivpu and vmwgfx Reviewed-by: Andrzej Hajda Acked-by: Maxime Ripard Link: https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com Signed-off-by: Jani Nikula --- Cc: Jacek Lawrynowicz Cc: Stanislaw Gruszka Cc: Oded Gabbay Cc: Andrzej Hajda Cc: Neil Armstrong Cc: Robert Foss Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Cc: Jani Nikula Cc: Rodrigo Vivi Cc: Joonas Lahtinen Cc: Tvrtko Ursulin Cc: Karol Herbst Cc: Lyude Paul Cc: Danilo Krummrich Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: Huang Rui Cc: Zack Rusin Cc: Broadcom internal kernel review list Cc: dri-de...@lists.freedesktop.org Cc: intel-...@lists.freedesktop.org Cc: intel...@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Cc: amd-gfx@lists.freedesktop.org --- drivers/accel/ivpu/ivpu_debugfs.c| 2 ++ drivers/gpu/drm/bridge/panel.c | 2 ++ drivers/gpu/drm/drm_print.c | 6 +++--- drivers/gpu/drm/i915/display/intel_dmc.c | 1 + drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ drivers/gpu/drm/radeon/r100.c| 1 + drivers/gpu/drm/radeon/r300.c| 1 + drivers/gpu/drm/radeon/r420.c| 1 + drivers/gpu/drm/radeon/r600.c| 3 ++- drivers/gpu/drm/radeon/radeon_fence.c| 1 + drivers/gpu/drm/radeon/radeon_gem.c | 1 + drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ drivers/gpu/drm/radeon/radeon_pm.c | 1 + drivers/gpu/drm/radeon/radeon_ring.c | 2 ++ drivers/gpu/drm/radeon/radeon_ttm.c | 1 + drivers/gpu/drm/radeon/rs400.c | 1 + drivers/gpu/drm/radeon/rv515.c | 1 + drivers/gpu/drm/ttm/ttm_device.c | 1 + drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- drivers/gpu/drm/ttm/ttm_tt.c | 5 +++-- drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ include/drm/drm_print.h | 2 +- 22 files changed, 34 insertions(+), 8 deletions(-) diff --git a/drivers/accel/ivpu/ivpu_debugfs.c b/drivers/accel/ivpu/ivpu_debugfs.c index d09d29775b3f..e07e447d08d1 100644 --- a/drivers/accel/ivpu/ivpu_debugfs.c +++ b/drivers/accel/ivpu/ivpu_debugfs.c @@ -3,6 +3,8 @@ * Copyright (C) 2020-2023 Intel Corporation */ +#include + #include #include #include diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 7f41525f7a6e..32506524d9a2 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -4,6 +4,8 @@ * Copyright (C) 2017 Broadcom */ +#include + #include #include #include diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 699b7dbffd7b..cf2efb44722c 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -23,13 +23,13 @@ * Rob Clark */ -#include - +#include +#include #include #include #include #include -#include +#include #include #include diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c index a34ff3383fd3..370d61c7e342 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -22,6 +22,7 @@ * */ +#include #include #include "i915_drv.h" diff --git a/drivers/gpu/drm/nouveau/dispnv50/crc.c b/drivers/gpu/drm/nouveau/dispnv50/crc.c index 9c942fbd836d..5936b6b3b15d 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/crc.c +++ b/drivers/gpu/drm/nouveau/dispnv50/crc.c @@ -1,5 +1,7 @@ // SPDX-License-Identifier: MIT +#include #include + #include #include #include diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index 86b8b770af19..0b1e19345f43 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -26,6 +26,7 @@ * Jerome Glisse */ +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index 25201b9a5aae..1620f534f55f 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -26,6 +26,7 @@ * Jerome Glisse */ +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/r420.c b/drivers/gpu/drm/radeon/r420.c index eae8a6389f5e..a979662eaa73 100644 --- a/drivers/gpu/drm/radeon/r420.c +++ b/drivers/gpu/drm/radeon/r420.c @@ -26,6 +26,7 @@ * Jerome Glisse */ +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index b5e97d95a19f..087d41e370fd 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -26,11 +26,12 @@ * Jerome Glisse */ +#include #include #include #include -#include #include +#include #include #include diff --git a/drivers/gp
Re: [PATCH 1/2] drm/print: drop include debugfs.h and include where needed
On Wed, 10 Apr 2024, Jani Nikula wrote: > Surprisingly many places depend on debugfs.h to be included via > drm_print.h. Fix them. While all of this is trivial, merely adding some includes, please consider acking the changes to your corner of the kernel. Thanks, Jani. > > Signed-off-by: Jani Nikula > > --- > > Cc: Andrzej Hajda > Cc: Neil Armstrong > Cc: Robert Foss > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Jernej Skrabec > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Karol Herbst > Cc: Lyude Paul > Cc: Danilo Krummrich > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: Huang Rui > Cc: dri-de...@lists.freedesktop.org > Cc: intel-...@lists.freedesktop.org > Cc: intel...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: amd-gfx@lists.freedesktop.org > --- > drivers/gpu/drm/bridge/panel.c | 2 ++ > drivers/gpu/drm/drm_print.c | 6 +++--- > drivers/gpu/drm/i915/display/intel_dmc.c | 1 + > drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ > drivers/gpu/drm/radeon/r100.c| 1 + > drivers/gpu/drm/radeon/r300.c| 1 + > drivers/gpu/drm/radeon/r420.c| 1 + > drivers/gpu/drm/radeon/r600.c| 3 ++- > drivers/gpu/drm/radeon/radeon_fence.c| 1 + > drivers/gpu/drm/radeon/radeon_gem.c | 1 + > drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ > drivers/gpu/drm/radeon/radeon_pm.c | 1 + > drivers/gpu/drm/radeon/radeon_ring.c | 2 ++ > drivers/gpu/drm/radeon/radeon_ttm.c | 1 + > drivers/gpu/drm/radeon/rs400.c | 1 + > drivers/gpu/drm/radeon/rv515.c | 1 + > drivers/gpu/drm/ttm/ttm_device.c | 1 + > drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- > drivers/gpu/drm/ttm/ttm_tt.c | 5 +++-- > include/drm/drm_print.h | 2 +- > 20 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 7f41525f7a6e..32506524d9a2 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -4,6 +4,8 @@ > * Copyright (C) 2017 Broadcom > */ > > +#include > + > #include > #include > #include > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > index 699b7dbffd7b..cf2efb44722c 100644 > --- a/drivers/gpu/drm/drm_print.c > +++ b/drivers/gpu/drm/drm_print.c > @@ -23,13 +23,13 @@ > * Rob Clark > */ > > -#include > - > +#include > +#include > #include > #include > #include > #include > -#include > +#include > > #include > #include > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c > b/drivers/gpu/drm/i915/display/intel_dmc.c > index e61e9c1b8947..84748add186a 100644 > --- a/drivers/gpu/drm/i915/display/intel_dmc.c > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c > @@ -22,6 +22,7 @@ > * > */ > > +#include > #include > > #include "i915_drv.h" > diff --git a/drivers/gpu/drm/nouveau/dispnv50/crc.c > b/drivers/gpu/drm/nouveau/dispnv50/crc.c > index 9c942fbd836d..5936b6b3b15d 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/crc.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/crc.c > @@ -1,5 +1,7 @@ > // SPDX-License-Identifier: MIT > +#include > #include > + > #include > #include > #include > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c > index 86b8b770af19..0b1e19345f43 100644 > --- a/drivers/gpu/drm/radeon/r100.c > +++ b/drivers/gpu/drm/radeon/r100.c > @@ -26,6 +26,7 @@ > * Jerome Glisse > */ > > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c > index 25201b9a5aae..1620f534f55f 100644 > --- a/drivers/gpu/drm/radeon/r300.c > +++ b/drivers/gpu/drm/radeon/r300.c > @@ -26,6 +26,7 @@ > * Jerome Glisse > */ > > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/radeon/r420.c b/drivers/gpu/drm/radeon/r420.c > index eae8a6389f5e..a979662eaa73 100644 > --- a/drivers/gpu/drm/radeon/r420.c > +++ b/drivers/gpu/drm/radeon/r420.c > @@ -26,6 +26,7 @@ > * Jerome Glisse > */ > > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c > index b5e97d95a19f..087d41e370fd 100644 > --- a/drivers/gpu/drm/radeon/r600.c > +++ b/drivers/gpu/drm/radeon/r600.c &g
[PATCH 1/2] drm/print: drop include debugfs.h and include where needed
Surprisingly many places depend on debugfs.h to be included via drm_print.h. Fix them. Signed-off-by: Jani Nikula --- Cc: Andrzej Hajda Cc: Neil Armstrong Cc: Robert Foss Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Cc: Jani Nikula Cc: Karol Herbst Cc: Lyude Paul Cc: Danilo Krummrich Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: Huang Rui Cc: dri-de...@lists.freedesktop.org Cc: intel-...@lists.freedesktop.org Cc: intel...@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Cc: amd-gfx@lists.freedesktop.org --- drivers/gpu/drm/bridge/panel.c | 2 ++ drivers/gpu/drm/drm_print.c | 6 +++--- drivers/gpu/drm/i915/display/intel_dmc.c | 1 + drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ drivers/gpu/drm/radeon/r100.c| 1 + drivers/gpu/drm/radeon/r300.c| 1 + drivers/gpu/drm/radeon/r420.c| 1 + drivers/gpu/drm/radeon/r600.c| 3 ++- drivers/gpu/drm/radeon/radeon_fence.c| 1 + drivers/gpu/drm/radeon/radeon_gem.c | 1 + drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ drivers/gpu/drm/radeon/radeon_pm.c | 1 + drivers/gpu/drm/radeon/radeon_ring.c | 2 ++ drivers/gpu/drm/radeon/radeon_ttm.c | 1 + drivers/gpu/drm/radeon/rs400.c | 1 + drivers/gpu/drm/radeon/rv515.c | 1 + drivers/gpu/drm/ttm/ttm_device.c | 1 + drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- drivers/gpu/drm/ttm/ttm_tt.c | 5 +++-- include/drm/drm_print.h | 2 +- 20 files changed, 30 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 7f41525f7a6e..32506524d9a2 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -4,6 +4,8 @@ * Copyright (C) 2017 Broadcom */ +#include + #include #include #include diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 699b7dbffd7b..cf2efb44722c 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -23,13 +23,13 @@ * Rob Clark */ -#include - +#include +#include #include #include #include #include -#include +#include #include #include diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c index e61e9c1b8947..84748add186a 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -22,6 +22,7 @@ * */ +#include #include #include "i915_drv.h" diff --git a/drivers/gpu/drm/nouveau/dispnv50/crc.c b/drivers/gpu/drm/nouveau/dispnv50/crc.c index 9c942fbd836d..5936b6b3b15d 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/crc.c +++ b/drivers/gpu/drm/nouveau/dispnv50/crc.c @@ -1,5 +1,7 @@ // SPDX-License-Identifier: MIT +#include #include + #include #include #include diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index 86b8b770af19..0b1e19345f43 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -26,6 +26,7 @@ * Jerome Glisse */ +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index 25201b9a5aae..1620f534f55f 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -26,6 +26,7 @@ * Jerome Glisse */ +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/r420.c b/drivers/gpu/drm/radeon/r420.c index eae8a6389f5e..a979662eaa73 100644 --- a/drivers/gpu/drm/radeon/r420.c +++ b/drivers/gpu/drm/radeon/r420.c @@ -26,6 +26,7 @@ * Jerome Glisse */ +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index b5e97d95a19f..087d41e370fd 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -26,11 +26,12 @@ * Jerome Glisse */ +#include #include #include #include -#include #include +#include #include #include diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 9ebe4a0b9a6c..4fb780d96f32 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -30,6 +30,7 @@ */ #include +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 3fec3acdaf28..2ef201a072f1 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -26,6 +26,7 @@ * Jerome Glisse */ +#include #include #include diff --git a/drivers/gpu/drm/radeon/radeon_ib.c b/drivers/gpu/drm/radeon/radeon_ib.c index fb9ecf5dbe2b..63d914f3414d 100644 --- a/drivers/gpu/drm/radeon/radeon_ib.c +++ b/drivers/gpu/drm/rad
Re: [PATCH v0 02/14] drm/amdgpu,drm/radeon: Make I2C terminology more inclusive
On Wed, 03 Apr 2024, Ville Syrjälä wrote: > On Fri, Mar 29, 2024 at 06:38:10PM +0100, Andi Shyti wrote: >> Hi, >> >> On Fri, Mar 29, 2024 at 10:28:14AM -0700, Easwar Hariharan wrote: >> > On 3/29/2024 10:16 AM, Andi Shyti wrote: >> > > Hi Easwar, >> > > >> > > On Fri, Mar 29, 2024 at 05:00:26PM +, Easwar Hariharan wrote: >> > >> I2C v7, SMBus 3.2, and I3C specifications have replaced "master/slave" >> > > >> > > I don't understand why we forget that i3c is 1.1.1 :-) >> > >> > That's because it's a copy-paste error from Wolfram's cover letter. :) >> > I'll update >> > next go-around. >> >> not a binding comment, though. Just for completeness, because we >> are giving the version to the i2c and smbus, but not i3c. >> >> > >> with more appropriate terms. Inspired by and following on to Wolfram's >> > >> series to fix drivers/i2c/[1], fix the terminology for users of >> > >> I2C_ALGOBIT bitbanging interface, now that the approved verbiage exists >> > >> in the specification. >> > > >> > > The specification talks about: >> > > >> > > - master -> controller >> > > - slave -> target (and not client) >> > > >> > > But both you and Wolfram have used client. I'd like to reach >> > > some more consistency here. >> > >> > I had the impression that remote targets (i.e external to the device) were >> > to be called clients, >> > e.g. the QSFP FRUs in drivers/infiniband, and internal ones targets. >> > I chose the terminology according to that understanding, but now I can't >> > find where I got that >> > information. >> >> The word "client" does not even appear in the documentation (only >> one instance in the i3c document), so that the change is not >> related to the document as stated in the commit log. Unless, of >> course, I am missing something. >> >> I'm OK with choosing a "customized" naming, but we need to reach >> an agreement. >> >> I raised the same question to Wolfram. > > I don't know where that discussion happened, but my opinion > is NAK to "client". Life is already confusing enough with > these renames, so let's not make it even more confusing by > inventing new names nowhere to be found in the spec. > > And let's especially not invent names that don't even fit > the purpose. "Client" makes me think of "client/server" or > some real world analogy. Neither of which seem to have any > resemblence to how the term would be used for i2c. Agreed. I2C 7.0, I3C 1.1.1, and SMBus 3.2 have all switched to controller/target terminology. The SMBus spec has additionally converted generic host references to controller. At least for i915 where I have some say in the matter, controller/target it shall be. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v0 03/14] drm/gma500,drm/i915: Make I2C terminology more inclusive
On Tue, 02 Apr 2024, Easwar Hariharan wrote: > On 4/2/2024 7:32 AM, Jani Nikula wrote: >> On Tue, 02 Apr 2024, Easwar Hariharan wrote: >>> On 4/2/2024 12:48 AM, Jani Nikula wrote: >>>> On Fri, 29 Mar 2024, Easwar Hariharan wrote: >>>>> I2C v7, SMBus 3.2, and I3C specifications have replaced "master/slave" >>>>> with more appropriate terms. Inspired by and following on to Wolfram's >>>>> series to fix drivers/i2c/[1], fix the terminology for users of >>>>> I2C_ALGOBIT bitbanging interface, now that the approved verbiage exists >>>>> in the specification. >>>> >>>> gma500 and i915 changes should be split. See MAINTAINERS. >>>> >>>> Might also split the i915 changes to smaller pieces, it's kind of >>>> random. And the changes here are not strictly related to I2C AFAICT, so >>>> the commit message should be updated. >>>> >>>> BR, >>>> Jani. >>>> >>>> >>> >>> >>> >>> I will split gma500 and i915 into their respective patches if possible in >>> v2. >>> >>> Can you say more about the changes being "not strictly related to I2C"? My >>> heuristic was to grep for master/slave, and look in the surrounding context >>> for >>> i2c-related terminology (i2c_pin, 7-bit address, struct i2c_adapter, >>> i2c_bus, etc) >>> to confirm that they are i2c-related, then following the references around >>> to >>> make the compiler happy. For e.g., I did not change the many references to >>> bigjoiner >>> master and slave because I understood from context they were not i2c >>> references. >>> >>> A couple examples would help me restrict the changes to I2C, since as >>> mentioned in the >>> discussion on Wolfram's thread, there are places where migrating away from >>> master/slave >>> terms in the code would conflict with the original technical manuals and >>> reduce correlation >>> and understanding of the code. >> >> I guess I was looking at the VBT changes in intel_bios.c. Granted, they >> do end up being used as i2c addresses. No big deal. >> >> I think I'd expect the treewide i2c adapter changes to land first, via >> i2c, and subsequent cleanups to happen next, via individual driver >> trees. There's quite a bit of conflict potential merging this outside of >> drm-intel-next, and there's really no need for that. >> >> BR, >> Jani. >> > > Great! Just so I'm clear, do you still want the i915 changes split up more, > along with them being > split off from gma500? If we can merge the i915 changes via drm-intel-next, it's probably fine as a big i915 patch. Just the gma500 separated. (The struct i2c_algorithm change etc. necessarily has to go via I2C tree of course.) BR, Jani. > > Thanks, > Easwar -- Jani Nikula, Intel
Re: [PATCH v0 03/14] drm/gma500,drm/i915: Make I2C terminology more inclusive
On Tue, 02 Apr 2024, Easwar Hariharan wrote: > On 4/2/2024 12:48 AM, Jani Nikula wrote: >> On Fri, 29 Mar 2024, Easwar Hariharan wrote: >>> I2C v7, SMBus 3.2, and I3C specifications have replaced "master/slave" >>> with more appropriate terms. Inspired by and following on to Wolfram's >>> series to fix drivers/i2c/[1], fix the terminology for users of >>> I2C_ALGOBIT bitbanging interface, now that the approved verbiage exists >>> in the specification. >> >> gma500 and i915 changes should be split. See MAINTAINERS. >> >> Might also split the i915 changes to smaller pieces, it's kind of >> random. And the changes here are not strictly related to I2C AFAICT, so >> the commit message should be updated. >> >> BR, >> Jani. >> >> > > > > I will split gma500 and i915 into their respective patches if possible in v2. > > Can you say more about the changes being "not strictly related to I2C"? My > heuristic was to grep for master/slave, and look in the surrounding context > for > i2c-related terminology (i2c_pin, 7-bit address, struct i2c_adapter, i2c_bus, > etc) > to confirm that they are i2c-related, then following the references around to > make the compiler happy. For e.g., I did not change the many references to > bigjoiner > master and slave because I understood from context they were not i2c > references. > > A couple examples would help me restrict the changes to I2C, since as > mentioned in the > discussion on Wolfram's thread, there are places where migrating away from > master/slave > terms in the code would conflict with the original technical manuals and > reduce correlation > and understanding of the code. I guess I was looking at the VBT changes in intel_bios.c. Granted, they do end up being used as i2c addresses. No big deal. I think I'd expect the treewide i2c adapter changes to land first, via i2c, and subsequent cleanups to happen next, via individual driver trees. There's quite a bit of conflict potential merging this outside of drm-intel-next, and there's really no need for that. BR, Jani. > > Thanks, > Easwar > -- Jani Nikula, Intel
Re: [PATCH v0 03/14] drm/gma500,drm/i915: Make I2C terminology more inclusive
On Fri, 29 Mar 2024, Easwar Hariharan wrote:
> I2C v7, SMBus 3.2, and I3C specifications have replaced "master/slave"
> with more appropriate terms. Inspired by and following on to Wolfram's
> series to fix drivers/i2c/[1], fix the terminology for users of
> I2C_ALGOBIT bitbanging interface, now that the approved verbiage exists
> in the specification.
gma500 and i915 changes should be split. See MAINTAINERS.
Might also split the i915 changes to smaller pieces, it's kind of
random. And the changes here are not strictly related to I2C AFAICT, so
the commit message should be updated.
BR,
Jani.
>
> Compile tested, no functionality changes intended
>
> [1]:
> https://lore.kernel.org/all/20240322132619.6389-1-wsa+rene...@sang-engineering.com/
>
> Signed-off-by: Easwar Hariharan
> ---
> drivers/gpu/drm/gma500/cdv_intel_lvds.c | 2 +-
> drivers/gpu/drm/gma500/intel_bios.c | 22 +++---
> drivers/gpu/drm/gma500/intel_bios.h | 4 +--
> drivers/gpu/drm/gma500/intel_gmbus.c | 2 +-
> drivers/gpu/drm/gma500/psb_drv.h | 2 +-
> drivers/gpu/drm/gma500/psb_intel_drv.h| 2 +-
> drivers/gpu/drm/gma500/psb_intel_lvds.c | 4 +--
> drivers/gpu/drm/gma500/psb_intel_sdvo.c | 26
> drivers/gpu/drm/i915/display/dvo_ch7017.c | 14 -
> drivers/gpu/drm/i915/display/dvo_ch7xxx.c | 18 +--
> drivers/gpu/drm/i915/display/dvo_ivch.c | 16 +-
> drivers/gpu/drm/i915/display/dvo_ns2501.c | 18 +--
> drivers/gpu/drm/i915/display/dvo_sil164.c | 18 +--
> drivers/gpu/drm/i915/display/dvo_tfp410.c | 18 +--
> drivers/gpu/drm/i915/display/intel_bios.c | 22 +++---
> drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> .../gpu/drm/i915/display/intel_display_core.h | 2 +-
> drivers/gpu/drm/i915/display/intel_dsi.h | 2 +-
> drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 18 +--
> drivers/gpu/drm/i915/display/intel_dvo.c | 14 -
> drivers/gpu/drm/i915/display/intel_dvo_dev.h | 2 +-
> drivers/gpu/drm/i915/display/intel_gmbus.c| 4 +--
> drivers/gpu/drm/i915/display/intel_sdvo.c | 30 +--
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 4 +--
> drivers/gpu/drm/i915/gvt/edid.c | 28 -
> drivers/gpu/drm/i915/gvt/edid.h | 4 +--
> drivers/gpu/drm/i915/gvt/opregion.c | 2 +-
> 27 files changed, 150 insertions(+), 150 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> index f08a6803dc18..84c9122062c4 100644
> --- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> +++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
> @@ -565,7 +565,7 @@ void cdv_intel_lvds_init(struct drm_device *dev,
> dev->dev, "I2C bus registration failed.\n");
> goto err_encoder_cleanup;
> }
> - gma_encoder->i2c_bus->slave_addr = 0x2C;
> + gma_encoder->i2c_bus->client_addr = 0x2C;
> dev_priv->lvds_i2c_bus = gma_encoder->i2c_bus;
>
> /*
> diff --git a/drivers/gpu/drm/gma500/intel_bios.c
> b/drivers/gpu/drm/gma500/intel_bios.c
> index 8245b5603d2c..333bece1a64f 100644
> --- a/drivers/gpu/drm/gma500/intel_bios.c
> +++ b/drivers/gpu/drm/gma500/intel_bios.c
> @@ -14,8 +14,8 @@
> #include "psb_intel_drv.h"
> #include "psb_intel_reg.h"
>
> -#define SLAVE_ADDR1 0x70
> -#define SLAVE_ADDR2 0x72
> +#define CLIENT_ADDR10x70
> +#define CLIENT_ADDR20x72
>
> static void *find_section(struct bdb_header *bdb, int section_id)
> {
> @@ -357,10 +357,10 @@ parse_sdvo_device_mapping(struct drm_psb_private
> *dev_priv,
> /* skip the device block if device type is invalid */
> continue;
> }
> - if (p_child->slave_addr != SLAVE_ADDR1 &&
> - p_child->slave_addr != SLAVE_ADDR2) {
> + if (p_child->client_addr != CLIENT_ADDR1 &&
> + p_child->client_addr != CLIENT_ADDR2) {
> /*
> - * If the slave address is neither 0x70 nor 0x72,
> + * If the client address is neither 0x70 nor 0x72,
>* it is not a SDVO device. Skip it.
>*/
> continue;
> @@ -371,22 +371,22 @@ parse_sdvo_device_mapping(struct drm_psb_private
> *dev_priv,
> DRM_DEBUG_KMS("Incorrect SDVO port. Skip it\n");
> continue;
> }
> - DRM_DEBUG_KMS("the SDVO device with slave addr %2x is found on"
> + DRM_DEBUG_KMS("the SDVO device with client addr %2x is found on"
> " %s port\n",
> - p_child->slave_addr,
> + p_child->client_addr,
> (p_child->dvo
Re: [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
> + drm_edid_free(aconnector->edid);
>
> drm_connector_cleanup(connector);
> drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> @@ -298,15 +298,15 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> return drm_add_edid_modes(connector, NULL);
>
> if (!aconnector->edid) {
> - struct edid *edid;
> + const struct drm_edid *drm_edid;
>
> - edid = drm_dp_mst_get_edid(connector,
> &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> + drm_edid = drm_dp_mst_edid_read(connector,
> &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
>
> - if (!edid) {
> + if (!drm_edid) {
> amdgpu_dm_set_mst_status(&aconnector->mst_status,
> MST_REMOTE_EDID, false);
>
> - drm_connector_update_edid_property(
> + drm_edid_connector_update(
> &aconnector->base,
> NULL);
>
> @@ -340,7 +340,7 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> return ret;
> }
>
> - aconnector->edid = edid;
> + aconnector->edid = drm_edid;
> amdgpu_dm_set_mst_status(&aconnector->mst_status,
> MST_REMOTE_EDID, true);
> }
> @@ -355,10 +355,13 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> struct dc_sink_init_data init_params = {
> .link = aconnector->dc_link,
> .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
> + const struct edid *edid;
> +
> + edid = drm_edid_raw(aconnector->edid); // FIXME: Get rid of
> drm_edid_raw()
> dc_sink = dc_link_add_remote_sink(
> aconnector->dc_link,
> - (uint8_t *)aconnector->edid,
> - (aconnector->edid->extensions + 1) * EDID_LENGTH,
> + (uint8_t *)edid,
> + (edid->extensions + 1) * EDID_LENGTH,
> &init_params);
>
> if (!dc_sink) {
> @@ -412,10 +415,9 @@ static int dm_dp_mst_get_modes(struct drm_connector
> *connector)
> }
> }
>
> - drm_connector_update_edid_property(
> - &aconnector->base, aconnector->edid);
> + drm_edid_connector_update(&aconnector->base, aconnector->edid);
>
> - ret = drm_add_edid_modes(connector, aconnector->edid);
> + ret = drm_edid_connector_add_modes(connector);
>
> return ret;
> }
--
Jani Nikula, Intel
Re: [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
On Wed, 27 Mar 2024, Melissa Wen wrote: > 2. Most of the edid data handled by `dm_helpers_parse_edid_caps()` are >in drm_edid halpers, but there are still a few that are not managed by >them yet. For example: >``` > edid_caps->serial_number = edid_buf->serial; > edid_caps->manufacture_week = edid_buf->mfg_week; > edid_caps->manufacture_year = edid_buf->mfg_year; >``` >AFAIU I see the same pending issue in i915/pnpid_get_panel_type() - >that still uses drm_edid_raw(). See https://lore.kernel.org/r/cover.1711015462.git.jani.nik...@intel.com BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v5 7/8] drm/amd/display: Introduce KUnit tests to dc_dmub_srv library
On Thu, 22 Feb 2024, Rodrigo Siqueira wrote: > diff --git a/drivers/gpu/drm/amd/display/test/kunit/.kunitconfig > b/drivers/gpu/drm/amd/display/test/kunit/.kunitconfig > index eb6f81601757..4c5861ad58bd 100644 > --- a/drivers/gpu/drm/amd/display/test/kunit/.kunitconfig > +++ b/drivers/gpu/drm/amd/display/test/kunit/.kunitconfig > @@ -4,5 +4,6 @@ CONFIG_DRM=y > CONFIG_DRM_AMDGPU=y > CONFIG_DRM_AMD_DC=y > CONFIG_AMD_DC_BASICS_KUNIT_TEST=y > +CONFIG_AMD_DC_KUNIT_TEST=y > CONFIG_DCE_KUNIT_TEST=y > CONFIG_DML_KUNIT_TEST=y A bit random patch to comment on, but this hunk demonstrates the point: Should all the configs have DRM_AMD_ prefix to put them in a "namespace"? BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v2] drm/amd/display: add panel_power_savings sysfs entry to eDP connectors
On Thu, 15 Feb 2024, Mario Limonciello wrote: > I feel the solution to these concerns is that we should make a knob that > controls whether the DRM property is created or the sysfs file is > created but not let them happen simultaneously. *insert the eyeballs emoji here* I mean no matter what the problem is, this sounds like the solution is worse than the problem! BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On Thu, 15 Feb 2024, Ville Syrjälä wrote:
> On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote:
>> +static int
>> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
>> +{
>> +struct drm_connector *connector = data;
>> +struct drm_device *ddev = connector->dev;
>> +struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
>> +unsigned char start = block * EDID_LENGTH;
>> +void *edid;
>> +int r;
>> +
>> +if (!acpidev)
>> +return -ENODEV;
>> +
>> +switch (connector->connector_type) {
>> +case DRM_MODE_CONNECTOR_LVDS:
>> +case DRM_MODE_CONNECTOR_eDP:
>> +break;
>> +default:
>> +return -EINVAL;
>> +}
>
> We could have other types of connectors that want this too.
> I don't see any real benefit in having this check tbh. Drivers
> should simply notset the flag on connectors where it won't work,
> and only the driver can really know that.
Agreed.
>> const struct drm_edid *drm_edid_read(struct drm_connector *connector)
>> {
>> +const struct drm_edid *drm_edid = NULL;
>> +
>> if (drm_WARN_ON(connector->dev, !connector->ddc))
>> return NULL;
>>
>> -return drm_edid_read_ddc(connector, connector->ddc);
>> +if (connector->acpi_edid_allowed)
>
> That should probably be called 'prefer_acpi_edid' or something
> since it's the first choice when the flag is set.
>
> But I'm not so sure there's any real benefit in having this
> flag at all. You anyway have to modify the driver to use this,
> so why not just have the driver do the call directly instead of
> adding this extra detour via the flag?
Heh, round and round we go [1].
BR,
Jani.
[1] https://lore.kernel.org/r/87sf23auxv@intel.com
--
Jani Nikula, Intel
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
NULL);
> +
> + if (!edid) {
> + if (connector->force == DRM_FORCE_UNSPECIFIED &&
> !drm_probe_ddc(adapter))
> + return NULL;
> + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid,
> adapter, NULL);
> + }
>
> - edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter,
> NULL);
> drm_connector_update_edid_property(connector, edid);
> return edid;
> }
> EXPORT_SYMBOL(drm_get_edid);
>
> +/**
> + * drm_edid_read_acpi - get EDID data, if available
> + * @connector: connector we're probing
> + *
> + * Use the BIOS to attempt to grab EDID data if possible.
> + *
> + * The returned pointer must be freed using drm_edid_free().
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector)
> +{
> + const struct drm_edid *drm_edid;
> +
> + if (connector->force == DRM_FORCE_OFF)
> + return NULL;
> +
> + drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid,
> connector);
> +
> + /* Note: Do *not* call connector updates here. */
> +
> + return drm_edid;
> +}
> +EXPORT_SYMBOL(drm_edid_read_acpi);
> +
> /**
> * drm_edid_read_custom - Read EDID data using given EDID block read function
> * @connector: Connector to use
> @@ -2727,10 +2811,11 @@ const struct drm_edid *drm_edid_read_ddc(struct
> drm_connector *connector,
> EXPORT_SYMBOL(drm_edid_read_ddc);
>
> /**
> - * drm_edid_read - Read EDID data using connector's I2C adapter
> + * drm_edid_read - Read EDID data using BIOS or connector's I2C adapter
> * @connector: Connector to use
> *
> - * Read EDID using the connector's I2C adapter.
> + * Read EDID from BIOS if allowed by connector or by using the connector's
> + * I2C adapter.
> *
> * The EDID may be overridden using debugfs override_edid or firmware EDID
> * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this
> priority
> @@ -2742,10 +2827,18 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
> */
> const struct drm_edid *drm_edid_read(struct drm_connector *connector)
> {
> + const struct drm_edid *drm_edid = NULL;
> +
> if (drm_WARN_ON(connector->dev, !connector->ddc))
> return NULL;
>
> - return drm_edid_read_ddc(connector, connector->ddc);
> + if (connector->acpi_edid_allowed)
> + drm_edid = drm_edid_read_acpi(connector);
> +
> + if (!drm_edid)
> + drm_edid = drm_edid_read_ddc(connector, connector->ddc);
> +
> + return drm_edid;
> }
> EXPORT_SYMBOL(drm_edid_read);
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f..74ed47f37a69 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1886,6 +1886,12 @@ struct drm_connector {
>
> /** @hdr_sink_metadata: HDR Metadata Information read from sink */
> struct hdr_sink_metadata hdr_sink_metadata;
> +
> + /**
> + * @acpi_edid_allowed: Get the EDID from the BIOS, if available.
> + * This is only applicable to eDP and LVDS displays.
> + */
> + bool acpi_edid_allowed;
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 7923bc00dc7a..1c1ee927de9c 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -459,5 +459,6 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>
> const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
> int ext_id, int *ext_index);
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector);
>
> #endif /* __DRM_EDID_H__ */
--
Jani Nikula, Intel
Re: [PATCH v5 1/3] drm: Add support to get EDID from ACPI
edid *edid;
> + struct edid *edid = NULL;
>
> if (connector->force == DRM_FORCE_OFF)
> return NULL;
>
> - if (connector->force == DRM_FORCE_UNSPECIFIED &&
> !drm_probe_ddc(adapter))
> - return NULL;
> + if (connector->acpi_edid_allowed)
> + edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid,
> connector, NULL);
> +
> + if (!edid) {
> + if (connector->force == DRM_FORCE_UNSPECIFIED &&
> !drm_probe_ddc(adapter))
> + return NULL;
> + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid,
> adapter, NULL);
> + }
Hmm. So do you want the presence of ACPI EDID to determine whether the
display is there or not?
Note how the override and firmware EDID mechanisms only override the
EDID *contents*. They are orthogonal from connector forcing. So you can
override the EDID, but still rely on the DDC probe to determine if the
display is there.
The question is, how likely is it that the ACPI EDID is used not just
because the actual EDID is bogus, but also because the display just
doesn't respond to DDC at all?
If possible, I'd like ACPI EDID to follow the override/firmware EDID
semantics on this.
>
> - edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter,
> NULL);
> drm_connector_update_edid_property(connector, edid);
> return edid;
> }
> EXPORT_SYMBOL(drm_get_edid);
>
> +/**
> + * drm_edid_read_acpi - get EDID data, if available
> + * @connector: connector we're probing
> + *
> + * Use the BIOS to attempt to grab EDID data if possible.
> + *
> + * The returned pointer must be freed using drm_edid_free().
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector)
> +{
> + const struct drm_edid *drm_edid;
> +
> + if (connector->force == DRM_FORCE_OFF)
> + return NULL;
If the caller handled all the connector->force stuff, this could be
dropped.
> +
> + drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid,
> connector);
> +
> + /* Note: Do *not* call connector updates here. */
> +
> + return drm_edid;
> +}
> +EXPORT_SYMBOL(drm_edid_read_acpi);
> +
> /**
> * drm_edid_read_custom - Read EDID data using given EDID block read function
> * @connector: Connector to use
> @@ -2727,10 +2815,11 @@ const struct drm_edid *drm_edid_read_ddc(struct
> drm_connector *connector,
> EXPORT_SYMBOL(drm_edid_read_ddc);
>
> /**
> - * drm_edid_read - Read EDID data using connector's I2C adapter
> + * drm_edid_read - Read EDID data using BIOS or connector's I2C adapter
> * @connector: Connector to use
> *
> - * Read EDID using the connector's I2C adapter.
> + * Read EDID from BIOS if allowed by connector or by using the connector's
> + * I2C adapter.
> *
> * The EDID may be overridden using debugfs override_edid or firmware EDID
> * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this
> priority
> @@ -2742,10 +2831,18 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
> */
> const struct drm_edid *drm_edid_read(struct drm_connector *connector)
> {
> + const struct drm_edid *drm_edid = NULL;
> +
> if (drm_WARN_ON(connector->dev, !connector->ddc))
> return NULL;
>
> - return drm_edid_read_ddc(connector, connector->ddc);
> + if (connector->acpi_edid_allowed)
> + drm_edid = drm_edid_read_acpi(connector);
> +
> + if (!drm_edid)
> + drm_edid = drm_edid_read_ddc(connector, connector->ddc);
This should be in drm_edid_read_ddc() not drm_edid_read(). Please let's
not make the two behave any different. It would be really weird if one
had an ACPI check and the other not.
> +
> + return drm_edid;
> }
> EXPORT_SYMBOL(drm_edid_read);
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f..74ed47f37a69 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1886,6 +1886,12 @@ struct drm_connector {
>
> /** @hdr_sink_metadata: HDR Metadata Information read from sink */
> struct hdr_sink_metadata hdr_sink_metadata;
> +
> + /**
> + * @acpi_edid_allowed: Get the EDID from the BIOS, if available.
> + * This is only applicable to eDP and LVDS displays.
> + */
> + bool acpi_edid_allowed;
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 518d1b8106c7..38b5e1b5c773 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -463,5 +463,6 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>
> const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
> int ext_id, int *ext_index);
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector);
>
> #endif /* __DRM_EDID_H__ */
--
Jani Nikula, Intel
Re: [PATCH v4 1/3] drm: Add drm_get_acpi_edid() helper
On Sat, 10 Feb 2024, Mario Limonciello wrote: > On 2/9/2024 12:57, Daniel Vetter wrote: >> On Fri, Feb 09, 2024 at 09:34:13AM -0600, Mario Limonciello wrote: >>> On 2/9/2024 05:07, Daniel Vetter wrote: >>>> On Thu, Feb 08, 2024 at 11:57:11AM +0200, Jani Nikula wrote: >>>>> On Wed, 07 Feb 2024, Mario Limonciello wrote: >>>>>> Some manufacturers have intentionally put an EDID that differs from >>>>>> the EDID on the internal panel on laptops. Drivers can call this >>>>>> helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method. >>>>>> >>>>>> Signed-off-by: Mario Limonciello >>>>>> --- >>>>>>drivers/gpu/drm/Kconfig| 5 +++ >>>>>>drivers/gpu/drm/drm_edid.c | 77 ++ >>>>>>include/drm/drm_edid.h | 1 + >>>>>>3 files changed, 83 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>>>>> index 6ec33d36f3a4..ec2bb71e8b36 100644 >>>>>> --- a/drivers/gpu/drm/Kconfig >>>>>> +++ b/drivers/gpu/drm/Kconfig >>>>>> @@ -21,6 +21,11 @@ menuconfig DRM >>>>>> select KCMP >>>>>> select VIDEO_CMDLINE >>>>>> select VIDEO_NOMODESET >>>>>> +select ACPI_VIDEO if ACPI >>>>>> +select BACKLIGHT_CLASS_DEVICE if ACPI >>>>>> +select INPUT if ACPI >>>>>> +select X86_PLATFORM_DEVICES if ACPI && X86 >>>>>> +select ACPI_WMI if ACPI && X86 >>>>> >>>>> I think I'll defer to drm maintainers on whether this is okay or >>>>> something to be avoided. >>>> >>>> Uh yeah this is a bit much, and select just messes with everything. Just >>>> #ifdef this in the code with a dummy alternative, if users configure their >>>> kernel without acpi but need it, they get to keep all the pieces. >>>> >>>> Alternatively make a DRM_ACPI_HELPERS symbol, but imo a Kconfig for every >>>> function is also not great. And just using #ifdef in the code also works >>>> for CONFIG_OF, which is exactly the same thing for platforms using dt to >>>> describe hw. >>>> >>>> Also I'd expect ACPI code to already provide dummy functions if ACPI is >>>> provided, so you probably dont even need all that much #ifdef in the code. >>>> >>>> What we defo cant do is select platform/hw stuff just because you enable >>>> CONFIG_DRM. >>>> -Sima >>> >>> The problem was with linking. I'll experiment with #ifdef for the next >>> version. >> >> Ah yes, if e.g. acpi is a module but drm is built-in then it will compile, >> but not link. >> >> You need >> >> depends on (ACPI || ACPI=n) >> >> for this. Looks a bit funny but works for all combinations. > > Nope; this fails at link time with this combination: > > CONFIG_ACPI=y > CONFIG_ACPI_VIDEO=m > CONFIG_DRM=y > > ld: drivers/gpu/drm/drm_edid.o: in function `drm_do_probe_acpi_edid': > drm_edid.c:(.text+0xd34): undefined reference to `acpi_video_get_edid' > make[5]: *** [scripts/Makefile.vmlinux:37: vmlinux] Error 1 > > So the logical solution is to try > depends on (ACPI_VIDEO || ACPI_VIDEO=n) > > But that leads me back to the rabbit hole of why I had the selects moved > to drm instead of drivers in the first place: > > drivers/gpu/drm/Kconfig:8:error: recursive dependency detected! > drivers/gpu/drm/Kconfig:8: symbol DRM depends on ACPI_VIDEO > drivers/acpi/Kconfig:213: symbol ACPI_VIDEO depends on > BACKLIGHT_CLASS_DEVICE > drivers/video/backlight/Kconfig:136:symbol BACKLIGHT_CLASS_DEVICE is > selected by DRM_RADEON > drivers/gpu/drm/radeon/Kconfig:3: symbol DRM_RADEON depends on DRM Generally speaking the root cause is using "select" instead of "depends on" in the first place. The excessive selects are just band-aid over that root cause. And if you try to convert some but not all the selects to depends ons, you'll get recursive dependencies. Quoting Documentation/kbuild/kconfig-language.rst: Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO
Re: Re: [PATCH v4 1/3] drm: Add drm_get_acpi_edid() helper
On Thu, 08 Feb 2024, Maxime Ripard wrote:
> On Thu, Feb 08, 2024 at 11:57:11AM +0200, Jani Nikula wrote:
>> On Wed, 07 Feb 2024, Mario Limonciello wrote:
>> > Some manufacturers have intentionally put an EDID that differs from
>> > the EDID on the internal panel on laptops. Drivers can call this
>> > helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method.
>> >
>> > Signed-off-by: Mario Limonciello
>> > ---
>> > drivers/gpu/drm/Kconfig| 5 +++
>> > drivers/gpu/drm/drm_edid.c | 77 ++
>> > include/drm/drm_edid.h | 1 +
>> > 3 files changed, 83 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> > index 6ec33d36f3a4..ec2bb71e8b36 100644
>> > --- a/drivers/gpu/drm/Kconfig
>> > +++ b/drivers/gpu/drm/Kconfig
>> > @@ -21,6 +21,11 @@ menuconfig DRM
>> >select KCMP
>> >select VIDEO_CMDLINE
>> >select VIDEO_NOMODESET
>> > + select ACPI_VIDEO if ACPI
>> > + select BACKLIGHT_CLASS_DEVICE if ACPI
>> > + select INPUT if ACPI
>> > + select X86_PLATFORM_DEVICES if ACPI && X86
>> > + select ACPI_WMI if ACPI && X86
>>
>> I think I'll defer to drm maintainers on whether this is okay or
>> something to be avoided.
>>
>>
>> >help
>> > Kernel-level support for the Direct Rendering Infrastructure (DRI)
>> > introduced in XFree86 4.0. If you say Y here, you need to select
>> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > index 923c4423151c..c649b4f9fd8e 100644
>> > --- a/drivers/gpu/drm/drm_edid.c
>> > +++ b/drivers/gpu/drm/drm_edid.c
>> > @@ -28,6 +28,7 @@
>> > * DEALINGS IN THE SOFTWARE.
>> > */
>> >
>> > +#include
>> > #include
>> > #include
>> > #include
>> > @@ -2188,6 +2189,49 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned
>> > int block, size_t len)
>> >return ret == xfers ? 0 : -1;
>> > }
>> >
>> > +/**
>> > + * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
>> > + * @data: struct drm_device
>> > + * @buf: EDID data buffer to be filled
>> > + * @block: 128 byte EDID block to start fetching from
>> > + * @len: EDID data buffer length to fetch
>> > + *
>> > + * Try to fetch EDID information by calling acpi_video_get_edid()
>> > function.
>> > + *
>> > + * Return: 0 on success or error code on failure.
>> > + */
>> > +static int
>> > +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t
>> > len)
>> > +{
>> > + struct drm_device *ddev = data;
>> > + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
>> > + unsigned char start = block * EDID_LENGTH;
>> > + void *edid;
>> > + int r;
>> > +
>> > + if (!acpidev)
>> > + return -ENODEV;
>> > +
>> > + /* fetch the entire edid from BIOS */
>> > + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
>> > + if (r < 0) {
>> > + DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
>> > + return -EINVAL;
>> > + }
>> > + if (len > r || start > r || start + len > r) {
>> > + r = -EINVAL;
>> > + goto cleanup;
>> > + }
>> > +
>> > + memcpy(buf, edid + start, len);
>> > + r = 0;
>> > +
>> > +cleanup:
>> > + kfree(edid);
>> > +
>> > + return r;
>> > +}
>> > +
>> > static void connector_bad_edid(struct drm_connector *connector,
>> > const struct edid *edid, int num_blocks)
>> > {
>> > @@ -2643,6 +2687,39 @@ struct edid *drm_get_edid(struct drm_connector
>> > *connector,
>> > }
>> > EXPORT_SYMBOL(drm_get_edid);
>> >
>> > +/**
>> > + * drm_get_acpi_edid - get EDID data, if available
>>
>> I'd prefer all the new EDID API to be named drm_edid_*. Makes a clean
>> break from the old API, and is more consistent.
>>
>> So perhaps drm_edid_read_acpi() to be in line with all the other struct
>> drm_edid based EDID reading functions.
>>
>> > + * @connector: connector we're probing
>> > + *
>> > + * Use the BI
Re: [PATCH v4 1/3] drm: Add drm_get_acpi_edid() helper
Connector to use > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 7923bc00dc7a..ca41be289fc6 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -410,6 +410,7 @@ struct edid *drm_do_get_edid(struct drm_connector > *connector, > void *data); > struct edid *drm_get_edid(struct drm_connector *connector, > struct i2c_adapter *adapter); > +const struct drm_edid *drm_get_acpi_edid(struct drm_connector *connector); There's a comment /* Interface based on struct drm_edid */ towards the end of the file, gathering all the new API under it. Other than that, LGTM, BR, Jani. > u32 drm_edid_get_panel_id(struct i2c_adapter *adapter); > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, >struct i2c_adapter *adapter); -- Jani Nikula, Intel
Re: [PATCH v3 2/5] drm: Add drm_get_acpi_edid() helper
using given EDID block read function > * @connector: Connector to use > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 518d1b8106c7..60fbdc06badc 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -412,6 +412,7 @@ struct edid *drm_do_get_edid(struct drm_connector > *connector, > void *data); > struct edid *drm_get_edid(struct drm_connector *connector, > struct i2c_adapter *adapter); > +struct edid *drm_get_acpi_edid(struct drm_connector *connector); > u32 drm_edid_get_panel_id(struct i2c_adapter *adapter); > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, >struct i2c_adapter *adapter); -- Jani Nikula, Intel
Re: [PATCH 3/6] drm/amdgpu: prefer snprintf over sprintf
On Fri, 12 Jan 2024, Alex Deucher wrote: > On Wed, Jan 10, 2024 at 12:39 PM Jani Nikula wrote: >> >> This will trade the W=1 warning -Wformat-overflow to >> -Wformat-truncation. This lets us enable -Wformat-overflow subsystem >> wide. >> >> Cc: Alex Deucher >> Cc: Christian König >> Cc: Pan, Xinhui >> Cc: amd-gfx@lists.freedesktop.org >> Signed-off-by: Jani Nikula > > Acked-by: Alex Deucher > > Feel free to take this via whichever tree makes sense. Thanks, pushed this one patch to drm-misc-next as prep work. BR, Jani. > > Alex > >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> index b9674c57c436..82b4b2019fca 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >> @@ -329,7 +329,8 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev, >> >> ring->eop_gpu_addr = kiq->eop_gpu_addr; >> ring->no_scheduler = true; >> - sprintf(ring->name, "kiq_%d.%d.%d.%d", xcc_id, ring->me, ring->pipe, >> ring->queue); >> + snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", >> +xcc_id, ring->me, ring->pipe, ring->queue); >> r = amdgpu_ring_init(adev, ring, 1024, irq, >> AMDGPU_CP_KIQ_IRQ_DRIVER0, >> AMDGPU_RING_PRIO_DEFAULT, NULL); >> if (r) >> -- >> 2.39.2 >> -- Jani Nikula, Intel
Re: [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to
On Fri, 26 Jan 2024, Mario Limonciello wrote:
> On 1/26/2024 10:28, Melissa Wen wrote:
>> Hi,
>>
>> I'm debugging a null-pointer dereference when running
>> igt@kms_connector_force_edid and the way I found to solve the bug is to
>> stop using raw edid handler in amdgpu_connector_funcs_force and
>> create_eml_sink in favor of managing resouces via sruct drm_edid helpers
>> (Patch 1). The proper solution seems to be switch amdgpu_dm_connector
>> from struct edid to struct drm_edid and avoid the usage of different
>> approaches in the driver (Patch 2). However, doing it implies a good
>> amount of work and validation, therefore I decided to send this RFC
>> first to collect opinions and check if there is any parallel work on
>> this side. It's a working in progress.
>>
>> The null-pointer error trigger by the igt@kms_connector_force_edid test
>> was introduced by:
>> - e54ed41620f ("drm/amd/display: Remove unwanted drm edid references")
>>
>> You can check the error trace in the first patch.
>>
>> This series was tested with kms_hdmi_inject and kms_force_connector. No
>> null-pointer error, kms_hdmi_inject is successul and kms_force_connector
>> is sucessful after the second execution - the force-edid subtest
>> still fails in the first run (I'm still investigating).
>>
>> There is also a couple of cast warnings to be addressed - I'm looking
>> for the best replacement.
>>
>> I appreciate any feedback and testing.
>
> So I'm actually a little bit worried by hardcoding EDID_LENGTH in this
> series.
>
> I have some other patches that I'm posting later on that let you get the
> EDID from _DDC BIOS method too. My observation was that the EDID can be
> anywhere up to 512 bytes according to the ACPI spec.
>
> An earlier version of my patch was using EDID_LENGTH when fetching it
> and the EDID checksum failed.
>
> I'll CC you on the post, we probably want to get your changes and mine
> merged together.
One of the main points of struct drm_edid is that it tracks the
allocation size separately.
We should simply not trust edid->extensions, because most of the time it
originates from outside the kernel.
Using drm_edid and immediately drm_edid_raw() falls short. That function
should only be used during migration to help. And yeah, it also means
EDID parsing should be done in drm_edid.c, and not spread out all over
the subsystem.
BR,
Jani.
>
>>
>> Melissa
>>
>> Melissa Wen (2):
>>drm/amd/display: fix null-pointer dereference on edid reading
>>drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
>>
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 78 ++-
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +-
>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 9 ++-
>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 23 +++---
>> 4 files changed, 60 insertions(+), 54 deletions(-)
>>
>
--
Jani Nikula, Intel
Re: [PATCH 2/2] drm/amd: Fetch the EDID from _DDC if available for eDP
On Mon, 29 Jan 2024, Mario Limonciello wrote: > On 1/29/2024 03:39, Jani Nikula wrote: >> On Fri, 26 Jan 2024, Mario Limonciello wrote: >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c >>> index 9caba10315a8..c7e1563a46d3 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c >>> @@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct >>> drm_connector *connector) >>> struct amdgpu_device *adev = drm_to_adev(dev); >>> struct amdgpu_connector *amdgpu_connector = >>> to_amdgpu_connector(connector); >>> >>> + if (amdgpu_connector->edid) >>> + return; >>> + >>> + /* if the BIOS specifies the EDID via _DDC, prefer this */ >>> + amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector); >> >> Imagine the EDID returned by acpi_video_get_edid() has edid->extensions >> bigger than 4. Of course it should not, but you have no guarantees, and >> it originates outside of the kernel. >> >> The real fix is to have the function return a struct drm_edid which >> tracks the allocation size separately. Unfortunately, it requires a >> bunch of changes along the way. We've mostly done it in i915, and I've >> sent a series to do this in drm/bridge [1]. Looking at it again, perhaps the ACPI code should just return a blob, and the drm code should have a helper to wrap that around struct drm_edid, so that the ACPI code does not have to depend on drm. Basic idea remains. >> Bottom line, we should stop using struct edid in drivers. They'll all >> parse the info differently, and from what I've seen, often wrong. >> >> > > Thanks for the feedback. In that case this specific change should > probably rebase on the Melissa's work > https://lore.kernel.org/amd-gfx/20240126163429.56714-1-m...@igalia.com/ > after she takes into account the feedback. > > Let me ask you this though - do you think that after that's done should > we let all drivers get EDID from BIOS as a priority? Or would you > prefer that this is unique to amdgpu? If the reason for having this is that the panel EDID contains some garbage, that's certainly not unique to amdgpu... :p > Something like: > > 1) If user specifies on kernel command line and puts an EDID in > /lib/firmware use that. > 2) If BIOS has EDID in _DDC and it's eDP panel, use that. I think we should also look into this. We currently don't do this, and it might help with some machines. However, gut feeling says it's probably better to keep this as a per driver decision instead of trying to bolt it into drm helpers. BR, Jani. > 3) Get panel EDID. > -- Jani Nikula, Intel
Re: [PATCH 2/2] drm/amd: Fetch the EDID from _DDC if available for eDP
a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index cd98b3565178..1faa21f542bd 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6562,17 +6562,23 @@ static void amdgpu_dm_connector_funcs_force(struct
> drm_connector *connector)
> {
> struct amdgpu_dm_connector *aconnector =
> to_amdgpu_dm_connector(connector);
> struct amdgpu_connector *amdgpu_connector =
> to_amdgpu_connector(connector);
> + struct amdgpu_device *adev = drm_to_adev(connector->dev);
> struct dc_link *dc_link = aconnector->dc_link;
> struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
> struct edid *edid;
>
> + /* prefer ACPI over panel for eDP */
> + edid = amdgpu_acpi_edid(adev, connector);
> +
> /*
>* Note: drm_get_edid gets edid in the following order:
>* 1) override EDID if set via edid_override debugfs,
>* 2) firmware EDID if set via edid_firmware module parameter
>* 3) regular DDC read.
>*/
> - edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
> + if (!edid)
> + edid = drm_get_edid(connector,
> &amdgpu_connector->ddc_bus->aux.ddc);
> +
> if (!edid) {
> DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
> return;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index e3915c4f8566..6bf2a8867e76 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -895,6 +895,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
> {
> struct amdgpu_dm_connector *aconnector = link->priv;
> struct drm_connector *connector = &aconnector->base;
> + struct amdgpu_device *adev = drm_to_adev(connector->dev);
> struct i2c_adapter *ddc;
> int retry = 3;
> enum dc_edid_status edid_status;
> @@ -909,8 +910,10 @@ enum dc_edid_status dm_helpers_read_local_edid(
>* do check sum and retry to make sure read correct edid.
>*/
> do {
> -
> - edid = drm_get_edid(&aconnector->base, ddc);
> + /* prefer ACPI over panel for eDP */
> + edid = amdgpu_acpi_edid(adev, connector);
> + if (!edid)
> + edid = drm_get_edid(&aconnector->base, ddc);
>
> /* DP Compliance Test 4.2.2.6 */
> if (link->aux_mode && connector->edid_corrupt)
--
Jani Nikula, Intel
[PATCH 4/6] drm/amdgpu: remove amdgpu_connector_edid() and stop using edid_blob_ptr
amdgpu_connector_edid() copies the EDID from edid_blob_ptr as a side
effect if amdgpu_connector->edid isn't initialized. However, everywhere
that the returned EDID is used, the EDID should have been set
beforehands.
Only the drm EDID code should look at the EDID property, anyway, so stop
using it.
Cc: Alex Deucher
Cc: Christian König
Cc: Pan, Xinhui
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 16
drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h | 1 -
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 4 ++--
6 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 9caba10315a8..cae7479c3ecf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -246,22 +246,6 @@ amdgpu_connector_find_encoder(struct drm_connector
*connector,
return NULL;
}
-struct edid *amdgpu_connector_edid(struct drm_connector *connector)
-{
- struct amdgpu_connector *amdgpu_connector =
to_amdgpu_connector(connector);
- struct drm_property_blob *edid_blob = connector->edid_blob_ptr;
-
- if (amdgpu_connector->edid) {
- return amdgpu_connector->edid;
- } else if (edid_blob) {
- struct edid *edid = kmemdup(edid_blob->data, edid_blob->length,
GFP_KERNEL);
-
- if (edid)
- amdgpu_connector->edid = edid;
- }
- return amdgpu_connector->edid;
-}
-
static struct edid *
amdgpu_connector_get_hardcoded_edid(struct amdgpu_device *adev)
{
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
index 61fcef15ad72..eff833b6ed31 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.h
@@ -24,7 +24,6 @@
#ifndef __AMDGPU_CONNECTORS_H__
#define __AMDGPU_CONNECTORS_H__
-struct edid *amdgpu_connector_edid(struct drm_connector *connector);
void amdgpu_connector_hotplug(struct drm_connector *connector);
int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector);
u16 amdgpu_connector_encoder_get_dp_bridge_encoder_id(struct drm_connector
*connector);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 587ee632a3b8..d0ba782f5aea 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1297,7 +1297,7 @@ static void
dce_v10_0_audio_write_speaker_allocation(struct drm_encoder *encoder
return;
}
- sad_count =
drm_edid_to_speaker_allocation(amdgpu_connector_edid(connector), &sadb);
+ sad_count = drm_edid_to_speaker_allocation(amdgpu_connector->edid,
&sadb);
if (sad_count < 0) {
DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n",
sad_count);
sad_count = 0;
@@ -1367,7 +1367,7 @@ static void dce_v10_0_audio_write_sad_regs(struct
drm_encoder *encoder)
return;
}
- sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
+ sad_count = drm_edid_to_sad(amdgpu_connector->edid, &sads);
if (sad_count < 0)
DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
if (sad_count <= 0)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index f22ec27365bd..d540bef6f331 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1329,7 +1329,7 @@ static void
dce_v11_0_audio_write_speaker_allocation(struct drm_encoder *encoder
return;
}
- sad_count =
drm_edid_to_speaker_allocation(amdgpu_connector_edid(connector), &sadb);
+ sad_count = drm_edid_to_speaker_allocation(amdgpu_connector->edid,
&sadb);
if (sad_count < 0) {
DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n",
sad_count);
sad_count = 0;
@@ -1399,7 +1399,7 @@ static void dce_v11_0_audio_write_sad_regs(struct
drm_encoder *encoder)
return;
}
- sad_count = drm_edid_to_sad(amdgpu_connector_edid(connector), &sads);
+ sad_count = drm_edid_to_sad(amdgpu_connector->edid, &sads);
if (sad_count < 0)
DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
if (sad_count <= 0)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 4dbe9b3259b5..7112d184eaad 100644
--- a/drivers/gpu/
[PATCH 3/6] drm/radeon: remove radeon_connector_edid() and stop using edid_blob_ptr
radeon_connector_edid() copies the EDID from edid_blob_ptr as a side
effect if radeon_connector->edid isn't initialized. However, everywhere
that the returned EDID is used, the EDID should have been set
beforehands.
Only the drm EDID code should look at the EDID property, anyway, so stop
using it.
Cc: Alex Deucher
Cc: Christian König
Cc: Pan, Xinhui
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/radeon/radeon_audio.c | 7 ---
drivers/gpu/drm/radeon/radeon_connectors.c | 15 ---
drivers/gpu/drm/radeon/radeon_mode.h | 2 --
3 files changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
b/drivers/gpu/drm/radeon/radeon_audio.c
index e88c35d003c5..05ee473dcfbb 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -304,6 +304,7 @@ void radeon_audio_endpoint_wreg(struct radeon_device *rdev,
u32 offset,
static void radeon_audio_write_sad_regs(struct drm_encoder *encoder)
{
struct drm_connector *connector =
radeon_get_connector_for_encoder(encoder);
+ struct radeon_connector *radeon_connector =
to_radeon_connector(connector);
struct radeon_encoder *radeon_encoder = to_radeon_encoder(encoder);
struct cea_sad *sads;
int sad_count;
@@ -311,7 +312,7 @@ static void radeon_audio_write_sad_regs(struct drm_encoder
*encoder)
if (!connector)
return;
- sad_count = drm_edid_to_sad(radeon_connector_edid(connector), &sads);
+ sad_count = drm_edid_to_sad(radeon_connector->edid, &sads);
if (sad_count < 0)
DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
if (sad_count <= 0)
@@ -327,6 +328,7 @@ static void radeon_audio_write_sad_regs(struct drm_encoder
*encoder)
static void radeon_audio_write_speaker_allocation(struct drm_encoder *encoder)
{
struct drm_connector *connector =
radeon_get_connector_for_encoder(encoder);
+ struct radeon_connector *radeon_connector =
to_radeon_connector(connector);
struct radeon_encoder *radeon_encoder = to_radeon_encoder(encoder);
u8 *sadb = NULL;
int sad_count;
@@ -334,8 +336,7 @@ static void radeon_audio_write_speaker_allocation(struct
drm_encoder *encoder)
if (!connector)
return;
- sad_count =
drm_edid_to_speaker_allocation(radeon_connector_edid(connector),
- &sadb);
+ sad_count = drm_edid_to_speaker_allocation(radeon_connector->edid,
&sadb);
if (sad_count < 0) {
DRM_DEBUG("Couldn't read Speaker Allocation Data Block: %d\n",
sad_count);
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
b/drivers/gpu/drm/radeon/radeon_connectors.c
index 81b5c3c8f658..80879e946342 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -255,21 +255,6 @@ static struct drm_encoder *radeon_find_encoder(struct
drm_connector *connector,
return NULL;
}
-struct edid *radeon_connector_edid(struct drm_connector *connector)
-{
- struct radeon_connector *radeon_connector =
to_radeon_connector(connector);
- struct drm_property_blob *edid_blob = connector->edid_blob_ptr;
-
- if (radeon_connector->edid) {
- return radeon_connector->edid;
- } else if (edid_blob) {
- struct edid *edid = kmemdup(edid_blob->data, edid_blob->length,
GFP_KERNEL);
- if (edid)
- radeon_connector->edid = edid;
- }
- return radeon_connector->edid;
-}
-
static void radeon_connector_get_edid(struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h
b/drivers/gpu/drm/radeon/radeon_mode.h
index 59c4db13d90a..0d2f7785a099 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -704,8 +704,6 @@ extern u16
radeon_connector_encoder_get_dp_bridge_encoder_id(struct drm_connecto
extern bool radeon_connector_is_dp12_capable(struct drm_connector *connector);
extern int radeon_get_monitor_bpc(struct drm_connector *connector);
-extern struct edid *radeon_connector_edid(struct drm_connector *connector);
-
extern void radeon_connector_hotplug(struct drm_connector *connector);
extern int radeon_dp_mode_valid_helper(struct drm_connector *connector,
struct drm_display_mode *mode);
--
2.39.2
[PATCH 2/6] drm/radeon: convert to using is_hdmi and has_audio from display info
Prefer the parsed results for is_hdmi and has_audio in display info over
calling drm_detect_hdmi_monitor() and drm_detect_monitor_audio(),
respectively.
Cc: Alex Deucher
Cc: Christian König
Cc: Pan, Xinhui
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/radeon/atombios_encoders.c | 10 +-
drivers/gpu/drm/radeon/evergreen_hdmi.c| 5 ++---
drivers/gpu/drm/radeon/radeon_audio.c | 6 +++---
drivers/gpu/drm/radeon/radeon_connectors.c | 12 ++--
drivers/gpu/drm/radeon/radeon_display.c| 2 +-
drivers/gpu/drm/radeon/radeon_encoders.c | 4 ++--
6 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c
b/drivers/gpu/drm/radeon/atombios_encoders.c
index 6e537c5bd295..386fd5f0a762 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -701,7 +701,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
if (radeon_connector->use_digital &&
(radeon_connector->audio == RADEON_AUDIO_ENABLE))
return ATOM_ENCODER_MODE_HDMI;
- else if
(drm_detect_hdmi_monitor(radeon_connector_edid(connector)) &&
+ else if (connector->display_info.is_hdmi &&
(radeon_connector->audio == RADEON_AUDIO_AUTO))
return ATOM_ENCODER_MODE_HDMI;
else if (radeon_connector->use_digital)
@@ -720,7 +720,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
if (radeon_audio != 0) {
if (radeon_connector->audio == RADEON_AUDIO_ENABLE)
return ATOM_ENCODER_MODE_HDMI;
- else if
(drm_detect_hdmi_monitor(radeon_connector_edid(connector)) &&
+ else if (connector->display_info.is_hdmi &&
(radeon_connector->audio == RADEON_AUDIO_AUTO))
return ATOM_ENCODER_MODE_HDMI;
else
@@ -737,14 +737,14 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
if ((dig_connector->dp_sink_type ==
CONNECTOR_OBJECT_ID_DISPLAYPORT) ||
(dig_connector->dp_sink_type == CONNECTOR_OBJECT_ID_eDP)) {
if (radeon_audio != 0 &&
-
drm_detect_monitor_audio(radeon_connector_edid(connector)) &&
+ connector->display_info.has_audio &&
ASIC_IS_DCE4(rdev) && !ASIC_IS_DCE5(rdev))
return ATOM_ENCODER_MODE_DP_AUDIO;
return ATOM_ENCODER_MODE_DP;
} else if (radeon_audio != 0) {
if (radeon_connector->audio == RADEON_AUDIO_ENABLE)
return ATOM_ENCODER_MODE_HDMI;
- else if
(drm_detect_hdmi_monitor(radeon_connector_edid(connector)) &&
+ else if (connector->display_info.is_hdmi &&
(radeon_connector->audio == RADEON_AUDIO_AUTO))
return ATOM_ENCODER_MODE_HDMI;
else
@@ -755,7 +755,7 @@ atombios_get_encoder_mode(struct drm_encoder *encoder)
break;
case DRM_MODE_CONNECTOR_eDP:
if (radeon_audio != 0 &&
- drm_detect_monitor_audio(radeon_connector_edid(connector))
&&
+ connector->display_info.has_audio &&
ASIC_IS_DCE4(rdev) && !ASIC_IS_DCE5(rdev))
return ATOM_ENCODER_MODE_DP_AUDIO;
return ATOM_ENCODER_MODE_DP;
diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c
b/drivers/gpu/drm/radeon/evergreen_hdmi.c
index 681119c91d94..09dda114e218 100644
--- a/drivers/gpu/drm/radeon/evergreen_hdmi.c
+++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c
@@ -412,7 +412,7 @@ void evergreen_hdmi_enable(struct drm_encoder *encoder,
bool enable)
if (enable) {
struct drm_connector *connector =
radeon_get_connector_for_encoder(encoder);
- if (connector &&
drm_detect_monitor_audio(radeon_connector_edid(connector))) {
+ if (connector && connector->display_info.has_audio) {
WREG32(HDMI_INFOFRAME_CONTROL0 + dig->afmt->offset,
HDMI_AVI_INFO_SEND | /* enable AVI info frames */
HDMI_AVI_INFO_CONT | /* required for audio info
values to be updated */
@@ -450,8 +450,7 @@ void evergreen_dp_enable(struct drm_encoder *encoder, bool
enable)
if (!dig || !dig->afmt)
Re: [PATCH 3/6] drm/amdgpu: prefer snprintf over sprintf
On Fri, 12 Jan 2024, kernel test robot wrote:
> Hi Jani,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on drm-misc/drm-misc-next]
> [also build test WARNING on drm-intel/for-linux-next
> drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.7
> next-20240111]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-nouveau-acr-ga102-remove-unused-but-set-variable/20240111-014206
> base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> patch link:
> https://lore.kernel.org/r/fea7a52924f98b1ac24f4a7e6ba21d7754422430.1704908087.git.jani.nikula%40intel.com
> patch subject: [PATCH 3/6] drm/amdgpu: prefer snprintf over sprintf
> config: sparc64-allmodconfig
> (https://download.01.org/0day-ci/archive/20240112/202401121126.i9vgrvmb-...@intel.com/config)
> compiler: sparc64-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build):
> (https://download.01.org/0day-ci/archive/20240112/202401121126.i9vgrvmb-...@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot
> | Closes:
> https://lore.kernel.org/oe-kbuild-all/202401121126.i9vgrvmb-...@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c: In function
> 'amdgpu_gfx_kiq_init_ring':
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:61: warning: '%d' directive
>>> output may be truncated writing between 1 and 10 bytes into a region of
>>> size between 0 and 8 [-Wformat-truncation=]
> 332 | snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d",
> | ^~
>drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:50: note: directive argument
> in the range [0, 2147483647]
> 332 | snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d",
> | ^
>drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:9: note: 'snprintf' output
> between 12 and 41 bytes into a destination of size 16
> 332 | snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d",
> | ^~~
> 333 | xcc_id, ring->me, ring->pipe, ring->queue);
> | ~~
As the commit message says,
This will trade the W=1 warning -Wformat-overflow to
-Wformat-truncation. This lets us enable -Wformat-overflow subsystem
wide.
BR,
Jani.
>
>
> vim +332 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>
>306
>307int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev,
>308 struct amdgpu_ring *ring,
>309 struct amdgpu_irq_src *irq, int
> xcc_id)
>310{
>311struct amdgpu_kiq *kiq = &adev->gfx.kiq[xcc_id];
>312int r = 0;
>313
>314spin_lock_init(&kiq->ring_lock);
>315
>316ring->adev = NULL;
>317ring->ring_obj = NULL;
>318ring->use_doorbell = true;
>319ring->xcc_id = xcc_id;
>320ring->vm_hub = AMDGPU_GFXHUB(xcc_id);
>321ring->doorbell_index =
>322(adev->doorbell_index.kiq +
>323 xcc_id *
> adev->doorbell_index.xcc_doorbell_range)
>324<< 1;
>325
>326r = amdgpu_gfx_kiq_acquire(adev, ring, xcc_id);
>327if (r)
>328return r;
>329
>330ring->eop_gpu_addr = kiq->eop_gpu_addr;
>331ring->no_scheduler = true;
> > 332snprintf(ring->name, sizeof(ring->name),
> "kiq_%d.%d.%d.%d",
>333 xcc_id, ring->me, ring->pipe, ring->queue);
>334r = amdgpu_ring_init(adev, ring, 1024, irq,
> AMDGPU_CP_KIQ_IRQ_DRIVER0,
>335 AMDGPU_RING_PRIO_DEFAULT, NULL);
>336if (r)
>337dev_warn(adev->dev, "(%d) failed to init kiq
> ring\n", r);
>338
>339return r;
>340}
>341
--
Jani Nikula, Intel
[PATCH 3/6] drm/amdgpu: prefer snprintf over sprintf
This will trade the W=1 warning -Wformat-overflow to -Wformat-truncation. This lets us enable -Wformat-overflow subsystem wide. Cc: Alex Deucher Cc: Christian König Cc: Pan, Xinhui Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Jani Nikula --- drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c index b9674c57c436..82b4b2019fca 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c @@ -329,7 +329,8 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev, ring->eop_gpu_addr = kiq->eop_gpu_addr; ring->no_scheduler = true; - sprintf(ring->name, "kiq_%d.%d.%d.%d", xcc_id, ring->me, ring->pipe, ring->queue); + snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d", +xcc_id, ring->me, ring->pipe, ring->queue); r = amdgpu_ring_init(adev, ring, 1024, irq, AMDGPU_CP_KIQ_IRQ_DRIVER0, AMDGPU_RING_PRIO_DEFAULT, NULL); if (r) -- 2.39.2
[PATCH] drm/amd: include drm/drm_edid.h only where needed
Including drm_edid.h from amdgpu_mode.h causes the rebuild of literally hundreds of files when drm_edid.h is modified, while there are only a handful of files that actually need to include drm_edid.h. Signed-off-by: Jani Nikula --- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h| 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c| 1 + drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 1 + drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 1 + drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 1 + drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 1 + 6 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index 32fe05c810c6..3802ccdf6f55 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -32,7 +32,6 @@ #include #include -#include #include #include #include @@ -51,6 +50,7 @@ struct amdgpu_device; struct amdgpu_encoder; struct amdgpu_router; struct amdgpu_hpd; +struct edid; #define to_amdgpu_crtc(x) container_of(x, struct amdgpu_crtc, base) #define to_amdgpu_connector(x) container_of(x, struct amdgpu_connector, base) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c index db6fc0cb18eb..453a4b786cfc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0+ #include +#include #include #include diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c index 3ee219aa2891..7672abe6c140 100644 --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c @@ -28,6 +28,7 @@ #include +#include #include #include "amdgpu.h" #include "amdgpu_connectors.h" diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c index bb666cb7522e..587ee632a3b8 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c @@ -21,6 +21,7 @@ * */ +#include #include #include #include diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c index 7af277f61cca..f22ec27365bd 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c @@ -21,6 +21,7 @@ * */ +#include #include #include #include diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index b599efda3b19..6f8128130b62 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include "dm_services.h" #include "amdgpu.h" -- 2.39.2
[PATCH] drm/radeon: include drm/drm_edid.h only where needed
Including drm_edid.h from radeon_mode.h causes the rebuild of more than a hundred files when drm_edid.h is modified, while there are only a handful of files that actually need to include drm_edid.h. Signed-off-by: Jani Nikula --- drivers/gpu/drm/radeon/atombios_encoders.c | 1 + drivers/gpu/drm/radeon/dce3_1_afmt.c | 1 + drivers/gpu/drm/radeon/dce6_afmt.c | 1 + drivers/gpu/drm/radeon/evergreen.c | 1 + drivers/gpu/drm/radeon/evergreen_hdmi.c| 1 + drivers/gpu/drm/radeon/radeon_atombios.c | 1 + drivers/gpu/drm/radeon/radeon_audio.c | 1 + drivers/gpu/drm/radeon/radeon_audio.h | 4 +++- drivers/gpu/drm/radeon/radeon_combios.c| 1 + drivers/gpu/drm/radeon/radeon_encoders.c | 1 + drivers/gpu/drm/radeon/radeon_mode.h | 2 +- 11 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c index 4aca09cab4b8..6e537c5bd295 100644 --- a/drivers/gpu/drm/radeon/atombios_encoders.c +++ b/drivers/gpu/drm/radeon/atombios_encoders.c @@ -29,6 +29,7 @@ #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/dce3_1_afmt.c b/drivers/gpu/drm/radeon/dce3_1_afmt.c index e8fe239b9d79..324e9b765098 100644 --- a/drivers/gpu/drm/radeon/dce3_1_afmt.c +++ b/drivers/gpu/drm/radeon/dce3_1_afmt.c @@ -21,6 +21,7 @@ * OTHER DEALINGS IN THE SOFTWARE. */ #include +#include #include "radeon.h" #include "radeon_asic.h" diff --git a/drivers/gpu/drm/radeon/dce6_afmt.c b/drivers/gpu/drm/radeon/dce6_afmt.c index 4a1d5447eac1..4c06f47453fd 100644 --- a/drivers/gpu/drm/radeon/dce6_afmt.c +++ b/drivers/gpu/drm/radeon/dce6_afmt.c @@ -21,6 +21,7 @@ * */ #include +#include #include "dce6_afmt.h" #include "radeon.h" diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index f0ae087be914..a424b86008b8 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -26,6 +26,7 @@ #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/radeon/evergreen_hdmi.c b/drivers/gpu/drm/radeon/evergreen_hdmi.c index 5f3078f8ab95..681119c91d94 100644 --- a/drivers/gpu/drm/radeon/evergreen_hdmi.c +++ b/drivers/gpu/drm/radeon/evergreen_hdmi.c @@ -26,6 +26,7 @@ */ #include +#include #include #include "evergreen_hdmi.h" #include "radeon.h" diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c index 85c4bb186203..3596ea4a8b60 100644 --- a/drivers/gpu/drm/radeon/radeon_atombios.c +++ b/drivers/gpu/drm/radeon/radeon_atombios.c @@ -27,6 +27,7 @@ #include #include +#include #include #include "radeon.h" diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c index 279bf130a18c..053058c5c1fa 100644 --- a/drivers/gpu/drm/radeon/radeon_audio.c +++ b/drivers/gpu/drm/radeon/radeon_audio.c @@ -26,6 +26,7 @@ #include #include +#include #include #include "dce6_afmt.h" #include "evergreen_hdmi.h" diff --git a/drivers/gpu/drm/radeon/radeon_audio.h b/drivers/gpu/drm/radeon/radeon_audio.h index 05e67867469b..dacaaa007051 100644 --- a/drivers/gpu/drm/radeon/radeon_audio.h +++ b/drivers/gpu/drm/radeon/radeon_audio.h @@ -27,7 +27,9 @@ #include -#define RREG32_ENDPOINT(block, reg)\ +struct cea_sad; + +#define RREG32_ENDPOINT(block, reg)\ radeon_audio_endpoint_rreg(rdev, (block), (reg)) #define WREG32_ENDPOINT(block, reg, v) \ radeon_audio_endpoint_wreg(rdev, (block), (reg), (v)) diff --git a/drivers/gpu/drm/radeon/radeon_combios.c b/drivers/gpu/drm/radeon/radeon_combios.c index 2620efc7c675..6952b1273b0f 100644 --- a/drivers/gpu/drm/radeon/radeon_combios.c +++ b/drivers/gpu/drm/radeon/radeon_combios.c @@ -28,6 +28,7 @@ #include #include +#include #include #include "radeon.h" diff --git a/drivers/gpu/drm/radeon/radeon_encoders.c b/drivers/gpu/drm/radeon/radeon_encoders.c index 9cb6401fe97e..3de3dce9e89d 100644 --- a/drivers/gpu/drm/radeon/radeon_encoders.c +++ b/drivers/gpu/drm/radeon/radeon_encoders.c @@ -26,6 +26,7 @@ #include +#include #include #include diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index 1decdcec0264..59c4db13d90a 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -32,13 +32,13 @@ #include #include -#include #include #include #include #include #include +struct edid; struct radeon_bo; struct radeon_device; -- 2.39.2
Re: [Intel-gfx] [PATCH v4 00/20] remove I2C_CLASS_DDC support
On Mon, 20 Nov 2023, Heiner Kallweit wrote: > v4: > - more ack and review tags Please do not send new versions just to record the acks and reviews. They should be added while applying the patches. Thanks, Jani. -- Jani Nikula, Intel
Re: [PATCH] drivers: gpu: Fix warning using plain integer as NULL
On Mon, 06 Nov 2023, Abhinav Singh wrote:
> On 11/6/23 16:53, Jani Nikula wrote:
>> On Fri, 03 Nov 2023, Abhinav Singh wrote:
>>> sparse static analysis tools generate a warning with this message
>>> "Using plain integer as NULL pointer". In this case this warning is
>>> being shown because we are trying to intialize a pointer to NULL using
>>> integer value 0.
>>>
>>> Signed-off-by: Abhinav Singh
>>> ---
>>> drivers/gpu/drm/radeon/clearstate_evergreen.h | 8
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/clearstate_evergreen.h
>>> b/drivers/gpu/drm/radeon/clearstate_evergreen.h
>>> index 63a1ffbb3ced..3b645558f133 100644
>>> --- a/drivers/gpu/drm/radeon/clearstate_evergreen.h
>>> +++ b/drivers/gpu/drm/radeon/clearstate_evergreen.h
>>> @@ -1049,7 +1049,7 @@ static const struct cs_extent_def SECT_CONTEXT_defs[]
>>> =
>>> {SECT_CONTEXT_def_5, 0xa29e, 5 },
>>> {SECT_CONTEXT_def_6, 0xa2a5, 56 },
>>> {SECT_CONTEXT_def_7, 0xa2de, 290 },
>>> -{ 0, 0, 0 }
>>> +{ NULL, 0, 0 }
>>
>> Random drive-by comment:
>>
>> I'd just use {} as the sentinel.
>>
>> BR,
>> Jani.
>>
>>> };
>>> static const u32 SECT_CLEAR_def_1[] =
>>> {
>>> @@ -1060,7 +1060,7 @@ static const u32 SECT_CLEAR_def_1[] =
>>> static const struct cs_extent_def SECT_CLEAR_defs[] =
>>> {
>>> {SECT_CLEAR_def_1, 0xffc0, 3 },
>>> -{ 0, 0, 0 }
>>> +{ NULL, 0, 0 }
>>> };
>>> static const u32 SECT_CTRLCONST_def_1[] =
>>> {
>>> @@ -1070,11 +1070,11 @@ static const u32 SECT_CTRLCONST_def_1[] =
>>> static const struct cs_extent_def SECT_CTRLCONST_defs[] =
>>> {
>>> {SECT_CTRLCONST_def_1, 0xf3fc, 2 },
>>> -{ 0, 0, 0 }
>>> +{ NULL, 0, 0 }
>>> };
>>> static const struct cs_section_def evergreen_cs_data[] = {
>>> { SECT_CONTEXT_defs, SECT_CONTEXT },
>>> { SECT_CLEAR_defs, SECT_CLEAR },
>>> { SECT_CTRLCONST_defs, SECT_CTRLCONST },
>>> -{ 0, SECT_NONE }
>>> +{ NULL, SECT_NONE }
>>> };
>>> --
>>> 2.39.2
>>>
>>
> Hi, Thanks for dropping by and the suggestion. I thought of using NULL
> instead of {} is because, first the warning itself says that 0 is used
> to intialize pointers with NULL, and second due this link
> https://www.spinics.net/lists/linux-sparse/msg10066.html where linus is
> talking about not using 0 NULL intialization of pointer variable and he
> thinks this is a legitimate issue and not some false positive
But... {} is neither of those things. It's empty initialization instead
of 0. It's valid in GCC and C23, and used all over the place in the
kernel.
BR,
Jani.
--
Jani Nikula, Intel
Re: [PATCH] drivers: gpu: Fix warning using plain integer as NULL
On Fri, 03 Nov 2023, Abhinav Singh wrote:
> sparse static analysis tools generate a warning with this message
> "Using plain integer as NULL pointer". In this case this warning is
> being shown because we are trying to intialize a pointer to NULL using
> integer value 0.
>
> Signed-off-by: Abhinav Singh
> ---
> drivers/gpu/drm/radeon/clearstate_evergreen.h | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/clearstate_evergreen.h
> b/drivers/gpu/drm/radeon/clearstate_evergreen.h
> index 63a1ffbb3ced..3b645558f133 100644
> --- a/drivers/gpu/drm/radeon/clearstate_evergreen.h
> +++ b/drivers/gpu/drm/radeon/clearstate_evergreen.h
> @@ -1049,7 +1049,7 @@ static const struct cs_extent_def SECT_CONTEXT_defs[] =
> {SECT_CONTEXT_def_5, 0xa29e, 5 },
> {SECT_CONTEXT_def_6, 0xa2a5, 56 },
> {SECT_CONTEXT_def_7, 0xa2de, 290 },
> -{ 0, 0, 0 }
> +{ NULL, 0, 0 }
Random drive-by comment:
I'd just use {} as the sentinel.
BR,
Jani.
> };
> static const u32 SECT_CLEAR_def_1[] =
> {
> @@ -1060,7 +1060,7 @@ static const u32 SECT_CLEAR_def_1[] =
> static const struct cs_extent_def SECT_CLEAR_defs[] =
> {
> {SECT_CLEAR_def_1, 0xffc0, 3 },
> -{ 0, 0, 0 }
> +{ NULL, 0, 0 }
> };
> static const u32 SECT_CTRLCONST_def_1[] =
> {
> @@ -1070,11 +1070,11 @@ static const u32 SECT_CTRLCONST_def_1[] =
> static const struct cs_extent_def SECT_CTRLCONST_defs[] =
> {
> {SECT_CTRLCONST_def_1, 0xf3fc, 2 },
> -{ 0, 0, 0 }
> +{ NULL, 0, 0 }
> };
> static const struct cs_section_def evergreen_cs_data[] = {
> { SECT_CONTEXT_defs, SECT_CONTEXT },
> { SECT_CLEAR_defs, SECT_CLEAR },
> { SECT_CTRLCONST_defs, SECT_CTRLCONST },
> -{ 0, SECT_NONE }
> +{ NULL, SECT_NONE }
> };
> --
> 2.39.2
>
--
Jani Nikula, Intel
