On Fri, Feb 06, 2015 at 05:38:55PM -0800, Bryce Harrington wrote: > On Fri, Feb 06, 2015 at 04:01:51PM -0800, Jon A. Cruz wrote: > > On 02/06/2015 03:50 PM, Bryce Harrington wrote: > > > On Fri, Feb 06, 2015 at 12:29:02PM -0800, Jon A. Cruz wrote: > > >> > > >> My initial concern with this is that it changes from one hardcoding to > > >> two hardcodings. Is 565 the only other format that will need to be > > >> supported? Could XRGB888 or others also be needed? > > >> > > >> If this needs changing it might be good to be sure it is comprehensive. > > > > > > Yes, that'd be nice, but couldn't those other formats be added as their > > > need crops up? > > > > If it were started as a solution using case, then yes. But by using the > > conditional operator the later changes would be more significant and > > allow for bugs to creep in. > > > > Also I do see a list of three formats set for some bitmask elsewhere. > > This then hints that the code cares about at least those three formats. > > Yeah I can see the case statement as more future proof. > > In any case, this is mesa code. Dongwon Kim, you'll need to submit this > to the mesa mailing list for inclusion. You should probably also give > consideration to Jon's review feedback. > > Bryce
Thanks for the feedback. I just came up with a new revision fo the patch and uploaded it to [email protected]. Can you guys visit there and review it? > > > >> On 02/03/2015 02:52 PM, Dongwon Kim wrote: > > >>> In current code, color format is always hardcoded to > > >>> __DRI_IMAGE_FORMAT_ARGB8888 > > >>> when buffer or DRI image is allocated in function calls, get_back_bo > > >>> and dri2_get_buffers, > > >>> regardless of current target's color format. This problem may leads to > > >>> incorrect render > > >>> pitch calculation, which eventually ends up with wrong offset of pixels > > >>> in the frame buffer > > >>> when the image is in different color format from dri surf's, especially > > >>> with different bpp. > > >>> (e.g. 16bpp) > > >>> > > >>> Attached code patch simply adds RGB565 case to two functions noted > > >>> above to resolve > > >>> the issue. > > >>> > > >>> Signed-off-by: Vivek Kasireddy <[email protected]> > > >>> Signed-off-by: Dongwon Kim <[email protected]> > > >>> --- > > >>> src/egl/drivers/dri2/platform_wayland.c | 10 ++++++++-- > > >>> 1 file changed, 8 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/src/egl/drivers/dri2/platform_wayland.c > > >>> b/src/egl/drivers/dri2/platform_wayland.c > > >>> index 3c34e07..78761e2 100644 > > >>> --- a/src/egl/drivers/dri2/platform_wayland.c > > >>> +++ b/src/egl/drivers/dri2/platform_wayland.c > > >>> @@ -293,6 +293,11 @@ get_back_bo(struct dri2_egl_surface *dri2_surf) > > >>> dri2_egl_display(dri2_surf->base.Resource.Display); > > >>> int i; > > >>> > > >>> + /* currently supports two DRI image formats, > > >>> __DRI_IMAGE_FORMAT_RGB565 or __DRI_IMAGE_FORMAT_ARGB8888, > > >>> + * one of which is selected depending on dri surface format */ > > >>> + const unsigned int dri_image_format = > > >>> + (dri2_surf->format == WL_DRM_FORMAT_RGB565) ? > > >>> __DRI_IMAGE_FORMAT_RGB565 : __DRI_IMAGE_FORMAT_ARGB8888; > > >>> + > > >>> /* We always want to throttle to some event (either a frame > > >>> callback or > > >>> * a sync request) after the commit so that we can be sure the > > >>> * compositor has had a chance to handle it and send us a release > > >>> event > > >>> @@ -322,7 +327,7 @@ get_back_bo(struct dri2_egl_surface *dri2_surf) > > >>> dri2_dpy->image->createImage(dri2_dpy->dri_screen, > > >>> dri2_surf->base.Width, > > >>> dri2_surf->base.Height, > > >>> - __DRI_IMAGE_FORMAT_ARGB8888, > > >>> + dri_image_format, > > >>> __DRI_IMAGE_USE_SHARE, > > >>> NULL); > > >>> dri2_surf->back->age = 0; > > >>> @@ -462,9 +467,10 @@ dri2_wl_get_buffers(__DRIdrawable * driDrawable, > > >>> unsigned int *attachments, int count, > > >>> int *out_count, void *loaderPrivate) > > >>> { > > >>> + struct dri2_egl_surface *dri2_surf = loaderPrivate; > > >>> unsigned int *attachments_with_format; > > >>> __DRIbuffer *buffer; > > >>> - const unsigned int format = 32; > > >>> + const unsigned int format = (dri2_surf->format == > > >>> WL_DRM_FORMAT_RGB565) ? 16 : 32; > > >>> int i; > > >>> > > >>> attachments_with_format = calloc(count, 2 * sizeof(unsigned int)); > > >>> > > >> _______________________________________________ > > >> wayland-devel mailing list > > >> [email protected] > > >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel > > > > -- > > Jon A. Cruz - Senior Open Source Developer > > Samsung Open Source Group > > [email protected] > > _______________________________________________ > > wayland-devel mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
