[PATCH] egl, wayland: RGB565 format support on Back-buffer

[dw kim]

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 mesa-dev at lists.freedesktop.org. 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 <vivek.kasireddy at intel.com>
> > >>> Signed-off-by: Dongwon Kim <dongwon.kim at intel.com>
> > >>> ---
> > >>>  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
> > >> wayland-devel at lists.freedesktop.org
> > >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > 
> > -- 
> > Jon A. Cruz - Senior Open Source Developer
> > Samsung Open Source Group
> > jonc at osg.samsung.com
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel