PERFORCE change 229059 for review

[Philip Withnall]

http://p4web.freebsd.org/@@229059?ac=10

Change 229059 by prw35 at pwithnall_zenith on 2013/05/27 15:23:08

	compositor: Optimise compositor_send_command()
	
	Eliminate some redundant bus reads and inline the function.
	
	This commit also fixes GetStatistics.

Affected files ...

.. //depot/projects/ctsrd/cheribsd/src/sys/dev/cheri/compositor/cheri_compositor_reg.c#3 edit

Differences ...

==== //depot/projects/ctsrd/cheribsd/src/sys/dev/cheri/compositor/cheri_compositor_reg.c#3 (text+ko) ====

@@ -182,7 +182,7 @@
 	 ((uint64_t) (address) & 0xfff) << 46 | \
 	 ((uint64_t) (entry) & 0x3fffffffffff))
 #define PAYLOAD_GET_STATISTICS(page) \
-	((uint64_t) (page) & 0x3)
+	((uint64_t) (page) & 0x7)
 #define PAYLOAD_CONTROL_STATISTICS(reset, isPaused) \
 	(((uint64_t) (reset) & 0x1) << 1 | \
 	 ((uint64_t) (isPaused) & 0x1))
@@ -203,14 +203,14 @@
 	uint64_t payload;
 } __packed;
 
-static void
+static inline void
 compositor_send_command(struct cheri_compositor_softc *sc,
     const struct compositor_command *command,
-    volatile struct compositor_response *response)
+    volatile struct compositor_response *response,
+    boolean_t expecting_response_payload)
 {
 	const uint32_t *command_raw = (const uint32_t *) command;
 	volatile uint32_t *response_raw = (volatile uint32_t *) response;
-	unsigned int i;
 
 	CHERI_COMPOSITOR_ASSERT_LOCKED(sc);
 
@@ -236,31 +236,31 @@
 
 	/* FIXME: Tidy up endianness; h64tole, le64toh etc. */
 
-	/* Poll for the response. FIXME: This should use interrupts.
-	 * Bail if this takes more than 100 attempts. */
-	i = 0;
-	do {
-		CHERI_COMPOSITOR_DEBUG(sc, "Checking for response…");
+	/* Read the response. The hardware guarantees that this is always
+	 * available on the next clock cycle. */
+	CHERI_COMPOSITOR_DEBUG(sc, "Checking for response…");
+
+	response_raw[0] =
+	    bus_read_8(sc->compositor_reg_res,
+	        CHERI_COMPOSITOR_RESPONSE_HEADER) & 0xffffffff;
 
-		response_raw[0] =
-		    bus_read_8(sc->compositor_reg_res,
-		        CHERI_COMPOSITOR_RESPONSE_HEADER) & 0xffffffff;
+	if (expecting_response_payload) {
+		/* This should get optimised if !expecting_response_payload. */
 		response->payload =
 		    bus_read_8(sc->compositor_reg_res,
 		        CHERI_COMPOSITOR_RESPONSE_PAYLOAD);
+	}
 
-		i++;
-	} while ((response->header.seq_num == 0 ||
-	          response->header.seq_num != command->header.seq_num) &&
-	         i <= 20);
-
-	/* Timeout? */
-	if (i >= 20) {
+#if defined(CHERI_COMPOSITOR_DEBUG_VERBOSE) && CHERI_COMPOSITOR_DEBUG_VERBOSE
+	/* Error? */
+	if (response->header.seq_num == 0 ||
+	    response->header.seq_num != command->header.seq_num) {
 		CHERI_COMPOSITOR_ERROR(sc,
-		    "Aborted sending compositor command due to timeout.");
+		    "Aborted sending compositor command due to error.");
 	}
 
 	CHERI_COMPOSITOR_DEBUG(sc, "Acknowledging response.");
+#endif
 
 	/* Acknowledge the response. */
 	bus_write_8(sc->compositor_reg_res,
@@ -930,7 +930,7 @@
 	command.payload =
 	    PAYLOAD_ALLOCATE_CFB(width_tiles, height_tiles,
 	        allocated_tiles_base);
-	compositor_send_command(sc, &command, &response);
+	compositor_send_command(sc, &command, &response, false);
 
 	if (response.header.status != STATUS_SUCCESS) {
 		retval = EIO;
@@ -1047,7 +1047,7 @@
 	        y * (MAX_X_RES / TILE_SIZE) + x /* address */,
 	        TILE_CACHE_ENTRY(is_opaque, tile_x_offset, tile_y_offset,
 	            allocated_tiles_base, width, height));
-	compositor_send_command(sc, &command, &response);
+	compositor_send_command(sc, &command, &response, false);
 
 	retval = (response.header.status == STATUS_SUCCESS) ? 0 : EIO;
 
@@ -1631,7 +1631,7 @@
 	command.payload =
 	    PAYLOAD_SET_CONFIGURATION(configuration->x_resolution,
 	       configuration->y_resolution);
-	compositor_send_command(sc, &command, &response);
+	compositor_send_command(sc, &command, &response, false);
 
 	if (response.header.status == STATUS_SUCCESS) {
 		/* Update our copies in memory. */
@@ -1815,7 +1815,7 @@
 	command.header.seq_num = sc->seq_num;
 	command.header.opcode = OPCODE_CONTROL_STATISTICS;
 	command.payload = PAYLOAD_CONTROL_STATISTICS(0, 1);
-	compositor_send_command(sc, &command, &response);
+	compositor_send_command(sc, &command, &response, false);
 
 	if (response.header.status != STATUS_SUCCESS) {
 		retval = EIO;
@@ -1858,7 +1858,7 @@
 	command.header.seq_num = sc->seq_num;
 	command.header.opcode = OPCODE_CONTROL_STATISTICS;
 	command.payload = PAYLOAD_CONTROL_STATISTICS(0, 0);
-	compositor_send_command(sc, &command, &response);
+	compositor_send_command(sc, &command, &response, false);
 
 	if (response.header.status != STATUS_SUCCESS) {
 		retval = EIO;
@@ -1902,7 +1902,7 @@
 	command.header.seq_num = sc->seq_num;
 	command.header.opcode = OPCODE_CONTROL_STATISTICS;
 	command.payload = PAYLOAD_CONTROL_STATISTICS(1, sc->is_sampler_paused);
-	compositor_send_command(sc, &command, &response);
+	compositor_send_command(sc, &command, &response, false);
 
 	if (response.header.status != STATUS_SUCCESS) {
 		retval = EIO;
@@ -1951,16 +1951,16 @@
 	command.header.seq_num = sc->seq_num;
 	command.header.opcode = OPCODE_GET_STATISTICS;
 	command.payload = PAYLOAD_GET_STATISTICS(page);
-	compositor_send_command(sc, &command, &response);
+	compositor_send_command(sc, &command, &response, true);
 
 	if (response.header.status != STATUS_SUCCESS) {
 		retval = EIO;
 		goto done;
 	}
 
-	/* Copy out the values. */
-	val1 = (response.payload >> 32) & 0xffff;
-	val2 = (response.payload >> 0) & 0xffff;
+	/* Copy out the values. Each is 29b long. */
+	val1 = (response.payload >> 32) & 0x1fffffff;
+	val2 = (response.payload >> 0) & 0x1fffffff;
 
 done:
 	*val1_out = val1;