| Message ID | 1457778185-22253-1-git-send-email-dhannawatpooja1@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/12/2016 03:23 AM, Pooja Dhannawat wrote: > As only DEPTH ==32 case is used, removing other dead code > which is based on DEPTH !== 32 > > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> > --- > +++ b/hw/display/omap_lcdc.c > @@ -71,44 +71,23 @@ 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" Umm, the old code WAS using DEPTH != 32. I'm not a display expert, so there may be justification in nuking that code; but the commit message needs a better argument than "the code wasn't used" when it sure seems to be used. If the argument is that surface_bits_per_pixel() always returns 32, that would be a good argument. > > static draw_line_func draw_line_table2[33] = { > [0 ... 32] = NULL, > - [8] = draw_line2_8, > - [15] = draw_line2_15, > - [16] = draw_line2_16, > [32] = draw_line2_32, This array is now wasteful. If surface_bits_per_pixel() always returns 32, then just ditch this array, and later on, change: /* Colour depth */ switch ((omap_lcd->palette[0] >> 12) & 7) { case 1: - draw_line = draw_line_table2[surface_bits_per_pixel(surface)]; + draw_line = draw_line2_32; bpp = 2; break; In other words, your cleanup, if justified, is incomplete.
On Wed, Mar 23, 2016 at 3:55 AM, Eric Blake <eblake@redhat.com> wrote: > On 03/12/2016 03:23 AM, Pooja Dhannawat wrote: > > As only DEPTH ==32 case is used, removing other dead code > > which is based on DEPTH !== 32 > > > > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> > > --- > > > +++ b/hw/display/omap_lcdc.c > > @@ -71,44 +71,23 @@ 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" > > Umm, the old code WAS using DEPTH != 32. I'm not a display expert, so > there may be justification in nuking that code; but the commit message > needs a better argument than "the code wasn't used" when it sure seems > to be used. If the argument is that surface_bits_per_pixel() always > returns 32, that would be a good argument. > > Correct me If I am wrong, I dig down on this and I found internally it used PIXMAN_FORMAT which uses bits_per_pixel = 32 only. > > > static draw_line_func draw_line_table2[33] = { > > [0 ... 32] = NULL, > > - [8] = draw_line2_8, > > - [15] = draw_line2_15, > > - [16] = draw_line2_16, > > [32] = draw_line2_32, > > This array is now wasteful. If surface_bits_per_pixel() always returns > 32, then just ditch this array, and later on, change: > > Yes sure. > /* Colour depth */ > switch ((omap_lcd->palette[0] >> 12) & 7) { > case 1: > - draw_line = draw_line_table2[surface_bits_per_pixel(surface)]; > + draw_line = draw_line2_32; > bpp = 2; > break; > > In other words, your cleanup, if justified, is incomplete. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
On Wed, Mar 23, 2016 at 11:35 AM, Pooja Dhannawat <dhannawatpooja1@gmail.com > wrote: > > > On Wed, Mar 23, 2016 at 3:55 AM, Eric Blake <eblake@redhat.com> wrote: > >> On 03/12/2016 03:23 AM, Pooja Dhannawat wrote: >> > As only DEPTH ==32 case is used, removing other dead code >> > which is based on DEPTH !== 32 >> > >> > Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> >> > --- >> >> > +++ b/hw/display/omap_lcdc.c >> > @@ -71,44 +71,23 @@ 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" >> >> Umm, the old code WAS using DEPTH != 32. I'm not a display expert, so >> there may be justification in nuking that code; but the commit message >> needs a better argument than "the code wasn't used" when it sure seems >> to be used. If the argument is that surface_bits_per_pixel() always >> returns 32, that would be a good argument. >> >> Correct me If I am wrong, I dig down on this and I found internally it > used PIXMAN_FORMAT which uses bits_per_pixel = 32 only. > > so surface_bits_per_pixel() always returns 32 (just for reference : Message-ID: <56F43A06.9090700@redhat.com> ) as discussed in this mail chain. I will make desired changed for this patch too. is it ok to go ahead with changes ? > > >> > static draw_line_func draw_line_table2[33] = { >> > [0 ... 32] = NULL, >> > - [8] = draw_line2_8, >> > - [15] = draw_line2_15, >> > - [16] = draw_line2_16, >> > [32] = draw_line2_32, >> >> This array is now wasteful. If surface_bits_per_pixel() always returns >> 32, then just ditch this array, and later on, change: >> >> Yes sure. > > >> /* Colour depth */ >> switch ((omap_lcd->palette[0] >> 12) & 7) { >> case 1: >> - draw_line = draw_line_table2[surface_bits_per_pixel(surface)]; >> + draw_line = draw_line2_32; >> bpp = 2; >> break; >> >> In other words, your cleanup, if justified, is incomplete. >> >> -- >> Eric Blake eblake redhat com +1-919-301-3266 >> Libvirt virtualization library http://libvirt.org >> >> >
diff --git a/hw/display/omap_lcd_template.h b/hw/display/omap_lcd_template.h index f0ce71f..1025ff3 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 @@ -152,7 +146,7 @@ static void glue(draw_line12_, DEPTH)(void *opaque, static void glue(draw_line16_, DEPTH)(void *opaque, uint8_t *d, const uint8_t *s, int width, int deststep) { -#if DEPTH == 16 && defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN) +#if defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN) memcpy(d, s, width * 2); #else uint16_t v; diff --git a/hw/display/omap_lcdc.c b/hw/display/omap_lcdc.c index ce1058b..4b3b810 100644 --- a/hw/display/omap_lcdc.c +++ b/hw/display/omap_lcdc.c @@ -71,44 +71,23 @@ 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" static draw_line_func draw_line_table2[33] = { [0 ... 32] = NULL, - [8] = draw_line2_8, - [15] = draw_line2_15, - [16] = draw_line2_16, [32] = draw_line2_32, }, draw_line_table4[33] = { [0 ... 32] = NULL, - [8] = draw_line4_8, - [15] = draw_line4_15, - [16] = draw_line4_16, [32] = draw_line4_32, }, draw_line_table8[33] = { [0 ... 32] = NULL, - [8] = draw_line8_8, - [15] = draw_line8_15, - [16] = draw_line8_16, [32] = draw_line8_32, }, draw_line_table12[33] = { [0 ... 32] = NULL, - [8] = draw_line12_8, - [15] = draw_line12_15, - [16] = draw_line12_16, [32] = draw_line12_32, }, draw_line_table16[33] = { [0 ... 32] = NULL, - [8] = draw_line16_8, - [15] = draw_line16_15, - [16] = draw_line16_16, [32] = draw_line16_32, };
As only DEPTH ==32 case is used, removing other dead code which is based on DEPTH !== 32 Signed-off-by: Pooja Dhannawat <dhannawatpooja1@gmail.com> --- hw/display/omap_lcd_template.h | 10 ++-------- hw/display/omap_lcdc.c | 21 --------------------- 2 files changed, 2 insertions(+), 29 deletions(-)