Re: [Qemu-devel] [PATCH] hw/display contains files named *_template.h. These are included many times with different values of the DEPTH macro. However, only the DEPTH == 32 case is used. Removed support for DEPTH != 32 in the template headers and in the file that include them.

[John Snow]

On 03/22/2016 02:34 AM, Nutan Shinde wrote:
> Signed-off-by: Nutan Shinde <address@hidden>
> ---
>  hw/display/blizzard.c          |  8 --------
>  hw/display/blizzard_template.h | 26 +-------------------------
>  hw/display/omap_lcd_template.h |  8 +-------
>  hw/display/omap_lcdc.c         |  6 ------
>  hw/display/sm501.c             | 17 -----------------
>  hw/display/sm501_template.h    |  8 +-------
>  6 files changed, 3 insertions(+), 70 deletions(-)
> 
> diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c
> index c231960..da5e133 100644
> --- a/hw/display/blizzard.c
> +++ b/hw/display/blizzard.c
> @@ -925,14 +925,6 @@ static void blizzard_update_display(void *opaque)
>      s->my[1] = 0;
>  }
>  
> -#define DEPTH 8
> -#include "blizzard_template.h"
> -#define DEPTH 15
> -#include "blizzard_template.h"
> -#define DEPTH 16
> -#include "blizzard_template.h"
> -#define DEPTH 24
> -#include "blizzard_template.h"
>  #define DEPTH 32
>  #include "blizzard_template.h"
>  
> diff --git a/hw/display/blizzard_template.h b/hw/display/blizzard_template.h
> index b7ef27c..a4c733d 100644
> --- a/hw/display/blizzard_template.h
> +++ b/hw/display/blizzard_template.h
> @@ -19,31 +19,7 @@
>   */
>  
>  #define SKIP_PIXEL(to)         (to += deststep)
> -#if DEPTH == 8
> -# define PIXEL_TYPE            uint8_t
> -# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
> -# define COPY_PIXEL1(to, from) (*to++ = from)
> -#elif DEPTH == 15 || DEPTH == 16
> -# define PIXEL_TYPE            uint16_t
> -# define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
> -# define COPY_PIXEL1(to, from) (*to++ = from)
> -#elif DEPTH == 24
> -# define PIXEL_TYPE            uint8_t
> -# define COPY_PIXEL(to, from) \
> -    do {                      \
> -        to[0] = from;         \
> -        to[1] = (from) >> 8;  \
> -        to[2] = (from) >> 16; \
> -        SKIP_PIXEL(to);       \
> -    } while (0)
> -
> -# define COPY_PIXEL1(to, from) \
> -    do {                       \
> -        *to++ = from;          \
> -        *to++ = (from) >> 8;   \
> -        *to++ = (from) >> 16;  \
> -    } while (0)
> -#elif DEPTH == 32
> +#if DEPTH == 32
>  # define PIXEL_TYPE            uint32_t
>  # define COPY_PIXEL(to, from)  do { *to = from; SKIP_PIXEL(to); } while (0)
>  # define COPY_PIXEL1(to, from) (*to++ = from)
> diff --git a/hw/display/omap_lcd_template.h b/hw/display/omap_lcd_template.h
> index f0ce71f..c1714ce 100644
> --- a/hw/display/omap_lcd_template.h
> +++ b/hw/display/omap_lcd_template.h
> @@ -27,13 +27,7 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> -#if DEPTH == 8
> -# define BPP 1
> -# define PIXEL_TYPE uint8_t
> -#elif DEPTH == 15 || DEPTH == 16
> -# define BPP 2
> -# define PIXEL_TYPE uint16_t
> -#elif DEPTH == 32
> +#if DEPTH == 32
>  # define BPP 4
>  # define PIXEL_TYPE uint32_t
>  #else
> diff --git a/hw/display/omap_lcdc.c b/hw/display/omap_lcdc.c
> index ce1058b..84b5f79 100644
> --- a/hw/display/omap_lcdc.c
> +++ b/hw/display/omap_lcdc.c
> @@ -71,12 +71,6 @@ static void omap_lcd_interrupts(struct omap_lcd_panel_s *s)
>  
>  #define draw_line_func drawfn
>  
> -#define DEPTH 8
> -#include "omap_lcd_template.h"
> -#define DEPTH 15
> -#include "omap_lcd_template.h"
> -#define DEPTH 16
> -#include "omap_lcd_template.h"
>  #define DEPTH 32
>  #include "omap_lcd_template.h"
>  
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 2957243..3fb64b5 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1170,23 +1170,6 @@ typedef void draw_line_func(uint8_t *d, const uint8_t 
> *s,
>  typedef void draw_hwc_line_func(SM501State * s, int crt, uint8_t * palette,
>                                  int c_y, uint8_t *d, int width);
>  
> -#define DEPTH 8
> -#include "sm501_template.h"
> -
> -#define DEPTH 15
> -#include "sm501_template.h"
> -
> -#define BGR_FORMAT
> -#define DEPTH 15
> -#include "sm501_template.h"
> -
> -#define DEPTH 16
> -#include "sm501_template.h"
> -
> -#define BGR_FORMAT
> -#define DEPTH 16
> -#include "sm501_template.h"
> -
>  #define DEPTH 32
>  #include "sm501_template.h"
>  
> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
> index f33e499..4e5801e 100644
> --- a/hw/display/sm501_template.h
> +++ b/hw/display/sm501_template.h
> @@ -22,13 +22,7 @@
>   * THE SOFTWARE.
>   */
>  
> -#if DEPTH == 8
> -#define BPP 1
> -#define PIXEL_TYPE uint8_t
> -#elif DEPTH == 15 || DEPTH == 16
> -#define BPP 2
> -#define PIXEL_TYPE uint16_t
> -#elif DEPTH == 32
> +#if DEPTH == 32
>  #define BPP 4
>  #define PIXEL_TYPE uint32_t
>  #else
> 

I can't comment on the patch itself (personally), but it looks like
maybe your patch submission got a little garbled.

The first line of your patch should be the subject line, which should be
limited to somewhere less than about 72 characters or so, following the
standard format of:

"<component>: <effect of patch>"

For example:

"display: remove redundant macro templates" or similar.

Then, the commit message should include your justification:

'hw/display contains files named *_template.h. These are included many
times with different values of the DEPTH macro. However, only the DEPTH
== 32 case is used. Removed support for DEPTH != 32 in the template
headers and in the file that include them.'

Please see:
http://wiki.qemu.org/Contribute/SubmitAPatch#Submitting_your_Patches


I'd recommend using the git format-patch and git send-email tools to
help remove any accidental error from the process.


Next, you need to make sure you CC the relevant maintainers for the
files you are patching, please see:
http://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer

In this case ... these files look a little neglected, with no obvious
maintainer. Looks like Peter Maydell is the only person to touch most of
these files in recent times, so maybe CC him.

Thanks,
--js