| Message ID | 1458843489-14104-1-git-send-email-dhannawatpooja1@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/24/2016 12:18 PM, Pooja Dhannawat wrote: > Removing support for DEPTH != 32 from blizzard template header > and file that includes it, as macro DEPTH == 32 only used. > > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> > --- > #define DEPTH 32 > #include "blizzard_template.h" > > + assert(surface_bits_per_pixel(surface) == 32) So this confirms that we only ever cared about DEPTH == 32. > + > + s->line_fn_tab[0] = blizzard_draw_fn_32; > + s->line_fn_tab[1] = blizzard_draw_fn_r_32; But now we are in the situation of having to look in the blizzard_template.h file... > +++ b/hw/display/blizzard_template.h > @@ -19,31 +19,7 @@ > */ > > #define SKIP_PIXEL(to) (to += deststep) > +#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) > @@ -58,9 +34,6 @@ > static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest, > const uint16_t *src, unsigned int width) ...and to reconstruct the results of the glue() macros to find the actual function definitions that we are using. I think this patch is okay as a first step, but I think we can go one step further in a followup patch: remove the glue() magic, delete blizzard_template.h, and just declare the used functions directly in blizzard.c. > @@ -74,7 +47,6 @@ static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest, > data >>= 5; > COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b)); And even without getting rid of the .h, there are some cleanups you can do now that DEPTH is a constant. For example, this line can shorten to: COPY_PIXEL1(dest, rgb_to_pixel32(r, g, b)); which in turn becomes a bit more legible as: *dest++ = rgb_to_pixel32(r, g, b); Since what you have is incrementally better, I'm fine if you add: Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, 03/24 23:48, Pooja Dhannawat wrote: > Removing support for DEPTH != 32 from blizzard template header > and file that includes it, as macro DEPTH == 32 only used. > > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> Hi Pooja, a meta-comment: in the future, when post a subsequent revision, please include what is changed since previous revision, in the cover letter if there are multiple patches, or under a "---" line in the commit message if there is no cover letter. (Remember that the Signed-off-by line must still stay above the "---" line). The reason to use "---" line is for maintainers to avoid "git am" applying the revision changelog into git history (apparently it is meaningless out of patch review context). In this case, it would be like: ----8<--- Removing support for DEPTH != 32 from blizzard template header and file that includes it, as macro DEPTH == 32 only used. Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> --- v4: Changed foo to bar, and baz to qux. [$name_of_suggester] ---->8---
On Fri, Mar 25, 2016 at 8:39 AM, Fam Zheng <famz@redhat.com> wrote: > On Thu, 03/24 23:48, Pooja Dhannawat wrote: > > Removing support for DEPTH != 32 from blizzard template header > > and file that includes it, as macro DEPTH == 32 only used. > > > > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> > > Hi Pooja, a meta-comment: in the future, when post a subsequent revision, > please include what is changed since previous revision, in the cover > letter if > there are multiple patches, or under a "---" line in the commit message if > there is no cover letter. (Remember that the Signed-off-by line must still > stay > above the "---" line). > > The reason to use "---" line is for maintainers to avoid "git am" applying > the > revision changelog into git history (apparently it is meaningless out of > patch > review context). > > In this case, it would be like: > > ----8<--- > > Removing support for DEPTH != 32 from blizzard template header > and file that includes it, as macro DEPTH == 32 only used. > > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> > > --- > > v4: Changed foo to bar, and baz to qux. [$name_of_suggester] > > ---->8--- > Thank you Fam. I will keep this in mind from onward.
diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c index c231960..5684e49 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" @@ -951,35 +943,10 @@ void *s1d13745_init(qemu_irq gpio_int) s->con = graphic_console_init(NULL, 0, &blizzard_ops, s); surface = qemu_console_surface(s->con); - switch (surface_bits_per_pixel(surface)) { - case 0: - s->line_fn_tab[0] = s->line_fn_tab[1] = - g_malloc0(sizeof(blizzard_fn_t) * 0x10); - break; - case 8: - s->line_fn_tab[0] = blizzard_draw_fn_8; - s->line_fn_tab[1] = blizzard_draw_fn_r_8; - break; - case 15: - s->line_fn_tab[0] = blizzard_draw_fn_15; - s->line_fn_tab[1] = blizzard_draw_fn_r_15; - break; - case 16: - s->line_fn_tab[0] = blizzard_draw_fn_16; - s->line_fn_tab[1] = blizzard_draw_fn_r_16; - break; - case 24: - s->line_fn_tab[0] = blizzard_draw_fn_24; - s->line_fn_tab[1] = blizzard_draw_fn_r_24; - break; - case 32: - s->line_fn_tab[0] = blizzard_draw_fn_32; - s->line_fn_tab[1] = blizzard_draw_fn_r_32; - break; - default: - fprintf(stderr, "%s: Bad color depth\n", __FUNCTION__); - exit(1); - } + assert(surface_bits_per_pixel(surface) == 32) + + s->line_fn_tab[0] = blizzard_draw_fn_32; + s->line_fn_tab[1] = blizzard_draw_fn_r_32; blizzard_reset(s); diff --git a/hw/display/blizzard_template.h b/hw/display/blizzard_template.h index b7ef27c..bc38d7a 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) @@ -58,9 +34,6 @@ static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest, const uint16_t *src, unsigned int width) { -#if !defined(SWAP_WORDS) && DEPTH == 16 - memcpy(dest, src, width); -#else uint16_t data; unsigned int r, g, b; const uint16_t *end = (const void *) src + width; @@ -74,7 +47,6 @@ static void glue(blizzard_draw_line16_, DEPTH)(PIXEL_TYPE *dest, data >>= 5; COPY_PIXEL1(dest, glue(rgb_to_pixel, DEPTH)(r, g, b)); } -#endif } static void glue(blizzard_draw_line24mode1_, DEPTH)(PIXEL_TYPE *dest,
Removing support for DEPTH != 32 from blizzard template header and file that includes it, as macro DEPTH == 32 only used. Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> --- hw/display/blizzard.c | 41 ++++------------------------------------- hw/display/blizzard_template.h | 30 +----------------------------- 2 files changed, 5 insertions(+), 66 deletions(-)