some minor dal cleanups and example of future patches

From: Dave Airlie <[email protected]>

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_services.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_services.c +0 -0

index 9d51259..9c852a3 100644

They are still used all over the place (e.g.
dc/dce110/dce110_resource.c:413).

We should at least do an spatch to use kzalloc/krealloc/kfree across the board if the wrappers are an issue.

NAKed

Harry

Quoted 25 lines · Dave Airlie

From: Dave Airlie <[email protected]>

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_services.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_services.c +0 -4

index 9d51259..9c852a3 100644
@@ -36,10 +36,6 @@
 #include "amdgpu_dm_types.h"
 #include "amdgpu_pm.h"
 
-#define dm_alloc(size) kzalloc(size, GFP_KERNEL)
-#define dm_realloc(ptr, size) krealloc(ptr, size, GFP_KERNEL)
-#define dm_free(ptr) kfree(ptr)
-
 
 /******************************************************************************
  * IRQ Interfaces.

Quoted 2 lines

*****************************************************************************/

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

From: Dave Airlie <[email protected]>

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_services.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_services.c +0 -4

index 9d51259..9c852a3 100644
@@ -36,10 +36,6 @@
 #include "amdgpu_dm_types.h"
 #include "amdgpu_pm.h"
 
-#define dm_alloc(size) kzalloc(size, GFP_KERNEL)
-#define dm_realloc(ptr, size) krealloc(ptr, size, GFP_KERNEL)
-#define dm_free(ptr) kfree(ptr)
-
 /**********************************************************
 ********************
  * IRQ Interfaces.

************************************************************
*****************/

_______________________________________________

They are still used all over the place (e.g.
dc/dce110/dce110_resource.c:413).

We should at least do an spatch to use kzalloc/krealloc/kfree across the board if the wrappers are an issue.

NAKed

They are still used all over the place (e.g.
dc/dce110/dce110_resource.c:413).

We should at least do an spatch to use kzalloc/krealloc/kfree across the board if the wrappers are an issue.

NAKed

Hi Emil,

It's a bit of a catch. The public (re: Linux) wants open, stable, efficient, and featureful drivers but then they want to dictate to the vendor exactly how that's going to be executed. That doesn't really work in practice or theory. The kernel team has standards that AMD has to meet but the process to get there won't always make everyone happy.

Intel has managed to get there

Hi Emil,

It's a bit of a catch.  The public (re: Linux) wants open, stable, efficient, and featureful drivers but then they want to dictate to the vendor exactly how that's going to be executed.  That doesn't really work in practice or theory.  The kernel team has standards that AMD has to meet but the process to get there won't always make everyone happy.

Quoted 1 line

Intel has managed to get there

I am not 100% sure.
E.g. http://www.phoronix.com/scan.php?page=article&item=intel-win10-mesa121&num=1

In general Intel driver under Linux is still behind MS Windows version.

Hi Harry,

Quoted 8 lines · Harry Wentland

They are still used all over the place (e.g.
dc/dce110/dce110_resource.c:413).

We should at least do an spatch to use kzalloc/krealloc/kfree across the board if the wrappers are an issue.

NAKed

[Ignoring all the heat around the DC/DAL topic for a moment]

Pardon for reviving this old thread, yet I believe it's beneficial, esp. for the less experienced open-source/kernel devs.

There is little-to-no merit of keeping dead code in upstream kernels. That is, unless there's a series in discussion which uses the code ;-)

NACKs like this one are not cool, furthermore the tend to alienate/push away people trying to help. With DC/DAL in mind, I think you/others want all the help people can offer ;-)

Regards,
Emil

From: Dave Airlie <[email protected]>

While I'm sure this is useful I think we should bring it back later.

It's usage of pid/tgid is incorrect, you have to get/put pid/tgids not store them away.

Signed-off-by Dave Airlie <[email protected]>

---
  .../drm/amd/display/amdgpu_dm/amdgpu_dm_services.c |  10 --
  drivers/gpu/drm/amd/display/dc/basics/Makefile     |   2 +-
  .../drm/amd/display/dc/basics/register_logger.c    | 197 ---------------------
  drivers/gpu/drm/amd/display/dc/dm_services.h       |  16 --
  .../drm/amd/display/include/dal_register_logger.h  |  42 -----
  5 files changed, 1 insertion(+), 266 deletions(-)

delete mode 100644 drivers/gpu/drm/amd/display/dc/basics/register_logger.c delete mode 100644 drivers/gpu/drm/amd/display/include/dal_register_logger.h

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_services.c +0 -10

index 9c852a3..565be05 100644
@@ -447,13 +447,3 @@ void dal_notify_setmode_complete(struct dc_context *ctx,
  	/*TODO*/
  }
  /* End of calls to notification */
-
-long dm_get_pid(void)
-{
-	return current->pid;
-}
-
-long dm_get_tgid(void)
-{
-	return current->tgid;
-}

drivers/gpu/drm/amd/display/dc/basics/Makefile +1 -1

index a263cad..0658162 100644
@@ -4,7 +4,7 @@
  # subcomponents.
 
  BASICS = conversion.o fixpt31_32.o fixpt32_32.o grph_object_id.o \
-	logger.o log_helpers.o register_logger.o signal_types.o vector.o
+	logger.o log_helpers.o signal_types.o vector.o
 
  AMD_DAL_BASICS = $(addprefix $(AMDDALPATH)/dc/basics/,$(BASICS))
 

drivers/gpu/drm/amd/display/dc/basics/register_logger.c deleted +0 -197

deleted file mode 100644
index b8d57d9..0000000
@@ -1,197 +0,0 @@
-/*
- * Copyright 2012-15 Advanced Micro Devices, Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- *
- * Authors: AMD
- *
- */
-
-#include "dm_services.h"
-#include "include/dal_types.h"
-#include "include/logger_interface.h"
-#include "logger.h"
-
-/******************************************************************************
- * Register Logger.
- * A facility to create register R/W logs.
- * Currently used for DAL Test.
- *****************************************************************************/
-
-/******************************************************************************
- * Private structures
- *****************************************************************************/
-struct dal_reg_dump_stack_location {
-	const char *current_caller_func;
-	long current_pid;
-	long current_tgid;
-	uint32_t rw_count;/* register access counter for current function. */
-};
-
-/* This the maximum number of nested calls to the 'reg_dump' facility. */
-#define DAL_REG_DUMP_STACK_MAX_SIZE 32
-
-struct dal_reg_dump_stack {
-	int32_t stack_pointer;
-	struct dal_reg_dump_stack_location
-		stack_locations[DAL_REG_DUMP_STACK_MAX_SIZE];
-	uint32_t total_rw_count; /* Total count for *all* functions. */
-};
-
-static struct dal_reg_dump_stack reg_dump_stack = {0};
-
-/******************************************************************************
- * Private functions
- *****************************************************************************/
-
-/* Check if current process is the one which requested register dump.
- * The reason for the check:
- * mmCRTC_STATUS_FRAME_COUNT is accessed by dal_controller_get_vblank_counter().
- * Which runs all the time when at least one display is connected.
- * (Triggered by drm_mode_page_flip_ioctl()). */
-static bool is_reg_dump_process(void)
-{
-	uint32_t i;
-
-	/* walk the list of our processes */
-	for (i = 0; i < reg_dump_stack.stack_pointer; i++) {
-		struct dal_reg_dump_stack_location *stack_location
-					= &reg_dump_stack.stack_locations[i];
-
-		if (stack_location->current_pid == dm_get_pid()
-			&& stack_location->current_tgid == dm_get_tgid())
-			return true;
-	}
-
-	return false;
-}
-
-static bool dal_reg_dump_stack_is_empty(void)
-{
-	if (reg_dump_stack.stack_pointer <= 0)
-		return true;
-	else
-		return false;
-}
-
-static struct dal_reg_dump_stack_location *dal_reg_dump_stack_push(void)
-{
-	struct dal_reg_dump_stack_location *current_location = NULL;
-
-	if (reg_dump_stack.stack_pointer >= DAL_REG_DUMP_STACK_MAX_SIZE) {
-		/* stack is full */
-		dm_output_to_console("[REG_DUMP]: %s: stack is full!\n",
-				__func__);
-	} else {
-		current_location =
-		&reg_dump_stack.stack_locations[reg_dump_stack.stack_pointer];
-		++reg_dump_stack.stack_pointer;
-	}
-
-	return current_location;
-}
-
-static struct dal_reg_dump_stack_location *dal_reg_dump_stack_pop(void)
-{
-	struct dal_reg_dump_stack_location *current_location = NULL;
-
-	if (dal_reg_dump_stack_is_empty()) {
-		/* stack is empty */
-		dm_output_to_console("[REG_DUMP]: %s: stack is empty!\n",
-				__func__);
-	} else {
-		--reg_dump_stack.stack_pointer;
-		current_location =
-		&reg_dump_stack.stack_locations[reg_dump_stack.stack_pointer];
-	}
-
-	return current_location;
-}
-
-/******************************************************************************
- * Public functions
- *****************************************************************************/
-
-void dal_reg_logger_push(const char *caller_func)
-{
-	struct dal_reg_dump_stack_location *free_stack_location;
-
-	free_stack_location = dal_reg_dump_stack_push();
-
-	if (NULL == free_stack_location)
-		return;
-
-	memset(free_stack_location, 0, sizeof(*free_stack_location));
-
-	free_stack_location->current_caller_func = caller_func;
-	free_stack_location->current_pid = dm_get_pid();
-	free_stack_location->current_tgid = dm_get_tgid();
-
-	dm_output_to_console("[REG_DUMP]:%s - start (pid:%ld, tgid:%ld)\n",
-		caller_func,
-		free_stack_location->current_pid,
-		free_stack_location->current_tgid);
-}
-
-void dal_reg_logger_pop(void)
-{
-	struct dal_reg_dump_stack_location *top_stack_location;
-
-	top_stack_location = dal_reg_dump_stack_pop();
-
-	if (NULL == top_stack_location) {
-		dm_output_to_console("[REG_DUMP]:%s - Stack is Empty!\n",
-				__func__);
-		return;
-	}
-
-	dm_output_to_console(
-	"[REG_DUMP]:%s - end."\
-	" Reg R/W Count: Total=%d Function=%d. (pid:%ld, tgid:%ld)\n",
-			top_stack_location->current_caller_func,
-			reg_dump_stack.total_rw_count,
-			top_stack_location->rw_count,
-			dm_get_pid(),
-			dm_get_tgid());
-
-	memset(top_stack_location, 0, sizeof(*top_stack_location));
-}
-
-void dal_reg_logger_rw_count_increment(void)
-{
-	++reg_dump_stack.total_rw_count;
-
-	++reg_dump_stack.stack_locations
-		[reg_dump_stack.stack_pointer - 1].rw_count;
-}
-
-bool dal_reg_logger_should_dump_register(void)
-{
-	if (true == dal_reg_dump_stack_is_empty())
-		return false;
-
-	if (false == is_reg_dump_process())
-		return false;
-
-	return true;
-}
-
-/******************************************************************************
- * End of File.
- *****************************************************************************/

drivers/gpu/drm/amd/display/dc/dm_services.h +0 -16

index 7a3f103..f3f9a40 100644
@@ -109,12 +109,6 @@ static inline uint32_t dm_read_reg_func(
 
  	value = cgs_read_register(ctx->cgs_device, address);
 
-#if defined(__DAL_REGISTER_LOGGER__)
-	if (true == dal_reg_logger_should_dump_register()) {
-		dal_reg_logger_rw_count_increment();
-		DRM_INFO("%s DC_READ_REG: 0x%x 0x%x\n", func_name, address, value);
-	}
-#endif
  	return value;
  }
 

@@ -127,13 +121,6 @@ static inline void dm_write_reg_func(
  	uint32_t value,
  	const char *func_name)
  {
-#if defined(__DAL_REGISTER_LOGGER__)
-	if (true == dal_reg_logger_should_dump_register()) {
-		dal_reg_logger_rw_count_increment();
-		DRM_INFO("%s DC_WRITE_REG: 0x%x 0x%x\n", func_name, address, value);
-	}
-#endif
-
  	if (address == 0) {
  		DC_ERR("invalid register write. address = 0");
  		return;

@@ -418,7 +405,4 @@ bool dm_dmcu_set_pipe(struct dc_context *ctx, unsigned int controller_id);
  #define dm_log_to_buffer(buffer, size, fmt, args)\
  	vsnprintf(buffer, size, fmt, args)
 
-long dm_get_pid(void);
-long dm_get_tgid(void);
-
  #endif /* __DM_SERVICES_H__ */

drivers/gpu/drm/amd/display/include/dal_register_logger.h deleted +0 -32

deleted file mode 100644
index 00dfcd7..0000000
@@ -1,42 +0,0 @@
-/*
- * Copyright 2012-15 Advanced Micro Devices, Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- *
- * Authors: AMD
- *
- */
-
-#ifndef __DAL_REGISTER_LOGGER__
-#define __DAL_REGISTER_LOGGER__
-
-/****************
- * API functions
- ***************/
-

So whenever I look at DAL I find myself trying to figure out what all of it does, and this leads me to deleting things, so I'm just sending out some stuff that I cleaned today.

One thing I noticed in passing is the displayport code seems possibly endian unsafe, it casts bytes read from hardware into bitfields, this is the best defined behaviour and is generally a pattern we try to steer away from.

The porting the displayport code to use drm defines is the sort of example of a cleanup patch that might cause problems for you in code sharing, but is the exact sort of patch that has been applied to other drivers in the past.

So whenever I look at DAL I find myself trying to figure out what all of it does, and this leads me to deleting things, so I'm just sending out some stuff that I cleaned today.

One thing I noticed in passing is the displayport code seems possibly endian unsafe, it casts bytes read from hardware into bitfields, this is the best defined behaviour and is generally a pattern we try to steer away from.

The porting the displayport code to use drm defines is the sort of example of a cleanup patch that might cause problems for you in code sharing, but is the exact sort of patch that has been applied to other drivers in the past.

Dave.

_______________________________________________
amd-gfx mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

So whenever I look at DAL I find myself trying to figure out what all of it does, and this leads me to deleting things, so I'm just sending out some stuff that I cleaned today.

One thing I noticed in passing is the displayport code seems possibly endian unsafe, it casts bytes read from hardware into bitfields, this is the best defined behaviour and is generally a pattern we try to steer away from.

The porting the displayport code to use drm defines is the sort of example of a cleanup patch that might cause problems for you in code sharing, but is the exact sort of patch that has been applied to other drivers in the past.

Dave.

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Quoted 4 lines · Dave Airlie

One thing I noticed in passing is the displayport code seems possibly endian unsafe, it casts bytes read from hardware into bitfields, this is the best defined behaviour and is generally a pattern we try to steer away from.

Yeah, we generally assume we run on x86. I haven't looked at all the bitfields lately but I do know that we generally use two definitions, one for little, one for big endian, for every bitfield.