[V1] sm501: dead code removal

From: Aishwarya Kadlag <aishwaryakadlag@gmail.com>

Remove support for DEPTH != 32 values from hw/display/*_template.h 
files and other files that include them. Only DEPTH == 32 case is 
supported.

Signed-off-by: Aishwarya Kadlag <aishwaryakadlag@gmail.com>
---
hw/display/sw501_1.c              |   36 ------------------------------------ 
hw/display/sw501_template_1.h     |    4 +++- 
2 files changed, 3 insertions(+), 37 deletions(-)

On 11.03.2018 17:40, Aishwarya Kadlag wrote:
> From: Aishwarya Kadlag <aishwaryakadlag@gmail.com>
> 
> Remove support for DEPTH != 32 values from hw/display/*_template.h 
> files and other files that include them. Only DEPTH == 32 case is 
> supported.
> 
> Signed-off-by: Aishwarya Kadlag <aishwaryakadlag@gmail.com>
> ---
> hw/display/sw501_1.c              |   36 ------------------------------------ 
> hw/display/sw501_template_1.h     |    4 +++- 
> 2 files changed, 3 insertions(+), 37 deletions(-)
> 
> 
> diff -u sm501.c sm501_1.c > sm501.patch -s

Patches should be applicable from the the top directory. It's best if
you create the patches with "git format-patch", see:

https://wiki.qemu.org/Contribute/SubmitAPatch#Use_git_format-patch

> --- sm501.c	2018-03-11 17:46:33.621452968 +0530
> +++ sm501_1.c	2018-03-11 17:53:26.933445566 +0530
> @@ -1358,22 +1358,6 @@
>                                  int width, const uint8_t *palette,
>                                  int c_x, int c_y);
>  
> -#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"
> @@ -1383,43 +1367,23 @@
>  #include "sm501_template.h"
>  
>  static draw_line_func *draw_line8_funcs[] = {
> -    draw_line8_8,
> -    draw_line8_15,
> -    draw_line8_16,
>      draw_line8_32,
>      draw_line8_32bgr,
> -    draw_line8_15bgr,
> -    draw_line8_16bgr,
>  };
>  
>  static draw_line_func *draw_line16_funcs[] = {
> -    draw_line16_8,
> -    draw_line16_15,
> -    draw_line16_16,
>      draw_line16_32,
>      draw_line16_32bgr,
> -    draw_line16_15bgr,
> -    draw_line16_16bgr,
>  };
>  
>  static draw_line_func *draw_line32_funcs[] = {
> -    draw_line32_8,
> -    draw_line32_15,
> -    draw_line32_16,
>      draw_line32_32,
>      draw_line32_32bgr,
> -    draw_line32_15bgr,
> -    draw_line32_16bgr,
>  };
>  
>  static draw_hwc_line_func *draw_hwc_line_funcs[] = {
> -    draw_hwc_line_8,
> -    draw_hwc_line_15,
> -    draw_hwc_line_16,
>      draw_hwc_line_32,
>      draw_hwc_line_32bgr,
> -    draw_hwc_line_15bgr,
> -    draw_hwc_line_16bgr,
>  };
>  
>  static inline int get_depth_index(DisplaySurface *surface)
> 
> 
> 
> diff -u sm501_template.h sm501_template_1.h > sm501_template.patch -s
> --- sm501_template.h    2018-03-11 17:25:36.816653718 +0530
> +++ sm501_template_1.h  2018-03-11 17:25:16.828654076 +0530
> @@ -22,13 +22,15 @@
>   * 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 don't know the hw/display code very well, but I think it's not that
easy... Putting Gerd on CC:, maybe he can describe the details of that
BiteSizeTask a little bit better.

(When sending patches, please also always use the get_maintainers.pl
script to get a set of people who should be put on CC: or the patch
might get lost in the high traffic of the qemu-devel mailing list).

 Thomas

Hi,

> I don't know the hw/display code very well, but I think it's not that
> easy... Putting Gerd on CC:, maybe he can describe the details of that
> BiteSizeTask a little bit better.

[ Side note: Is there a wiki page for this? ]

Might be it actually is that easy.

Historically qemu created DisplaySurfaces according to the needs of user
interface code, which could be all kinds of formats, and the rendering
code of the display emulation had to deal with it.

These days you always get 32bpp DisplaySurfaces unless you explicitly
do something else.  Code which uses qemu_console_resize() +
qemu_console_surface() or qemu_create_displaysurface() only need to
handle 32bpp rendering outout (PIXMAN_x8r8g8b8 to be exact, i.e. xRGB in
host native byte order).

But note that this applies to rendering only.  Sometimes there is pixel
bit shuffeling code which is part of the device emulation (the cirrus
blitter for example).  This must continue to support all formats of
course.

cheers,
  Gerd

On 12.03.2018 08:05, Gerd Hoffmann wrote:
>   Hi,
> 
>> I don't know the hw/display code very well, but I think it's not that
>> easy... Putting Gerd on CC:, maybe he can describe the details of that
>> BiteSizeTask a little bit better.
> 
> [ Side note: Is there a wiki page for this? ]

There is an entry here:

https://wiki.qemu.org/BiteSizedTasks#Dead_code_removal

Sorry Gerd, I thought it was likely you who added that task, but seems
like this specific task has rather been added by Paolo back in 2014 already:

https://wiki.qemu.org/index.php?title=Contribute/BiteSizedTasks&diff=4449&oldid=4444

 Thomas

On 12/03/2018 06:58, Thomas Huth wrote:
> I don't know the hw/display code very well, but I think it's not that
> easy... Putting Gerd on CC:, maybe he can describe the details of that
> BiteSizeTask a little bit better.
> 
> (When sending patches, please also always use the get_maintainers.pl
> script to get a set of people who should be put on CC: or the patch
> might get lost in the high traffic of the qemu-devel mailing list).

It's _almost_ that easy.

First, get_depth_index must be adjusted to reflect that you have removed
the first three elements from the array.  It doesn't hurt to add an

	assert(surface_bits_per_pixel(surface) == 32)

in the function, too.

Second, the PIXEL_TYPE macro in sm501_template.h now is always uint32_t
and it can be replaced everywhere.  Likewise for BPP, which is always 4,
and DEPTH, which is always 32.

Thanks!

Paolo

On 12 March 2018 at 07:05, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Might be it actually is that easy.

In fact looking through the list archives I see you actually
sent a patchset to do this for the sm501 device back in March
last year:
http://lists.gnu.org/archive/html/qemu-devel/2017-03/msg01172.html

I think it didn't get applied mostly because it was on list
at the same time as a significant revamp of the sm501 device
code. So it probably won't apply cleanly to the current code,
but it looks like a good starting point for identifying what
the changes needed here are.

thanks
-- PMM

Really glad to get a feedback so soon on my first attempt.

Yes Thomas sir, I will apply these changes in my next patch. Now that this
patch is already been implemented by Gerd sir in 2014 and it was not
applied, should I consider finding some other task or should I improve this
patch to get it merged or applied? Also as this was my first attempt, I
selected the task mentioned in the wiki page
https://wiki.qemu.org/BiteSizedTasks#Dead_code_removal
As Paolo sir said, if this is too easy then can someone please suggest me
any other task to work on so that I can create something useful to qemu??


Thanks,
Aishwarya



On Mon, Mar 12, 2018 at 6:33 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 12 March 2018 at 07:05, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > Might be it actually is that easy.
>
> In fact looking through the list archives I see you actually
> sent a patchset to do this for the sm501 device back in March
> last year:
> http://lists.gnu.org/archive/html/qemu-devel/2017-03/msg01172.html
>
> I think it didn't get applied mostly because it was on list
> at the same time as a significant revamp of the sm501 device
> code. So it probably won't apply cleanly to the current code,
> but it looks like a good starting point for identifying what
> the changes needed here are.
>
> thanks
> -- PMM
>