[14/32] drm/vmwgfx: Populate fb->pixel_format
diff mbox

Message ID 1479399271-31991-15-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä Nov. 17, 2016, 4:14 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Stuff something semi-reasonable into fb->pixel_format. I had to guess
as to which formats we should pick. Did I guess correctly?

We can't quite use drm_mode_legacy_fb_format() due to the ARGB1555
vs. XRGB155 mess. However use of 'A' formats should imply per-pixel
alpha blending as far as KMS is concerned so I'm not at all sure we
want to have any 'A' formats exposed as opposed to just 'X' formats.
OTOH vmvgfx doesn't do planes, and so the core won't enforce that
these formats match any list of supported formats, so the choice
shouldn't be super important at this point.

Cc: linux-graphics-maintainer@vmware.com
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Nov. 17, 2016, 5:52 p.m. UTC | #1
Hi Ville,

Thank you for the patch.

On Thursday 17 Nov 2016 18:14:13 ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Stuff something semi-reasonable into fb->pixel_format. I had to guess
> as to which formats we should pick. Did I guess correctly?
> 
> We can't quite use drm_mode_legacy_fb_format() due to the ARGB1555
> vs. XRGB155 mess. However use of 'A' formats should imply per-pixel
> alpha blending as far as KMS is concerned so I'm not at all sure we
> want to have any 'A' formats exposed as opposed to just 'X' formats.
> OTOH vmvgfx doesn't do planes, and so the core won't enforce that
> these formats match any list of supported formats, so the choice
> shouldn't be super important at this point.

The vmgfx driver should really be converted to using struct drm_mode_fb_cmd2 
through the whole code instead of converting struct drm_mode_fb_cmd2 to struct 
drm_mode_fb_cmd in vmw_kms_fb_create(). I gave it a go a few months back but 
the structure is passed around so many functions that conversion requires more 
knowledge about the driver than I have. I don't expect you to fix that, but it 
would be nice if Sinclair or Thomas could give it a go.

> Cc: linux-graphics-maintainer@vmware.com
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 7d92ab56961b..5788913ca8f9
> 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -524,6 +524,7 @@ static int vmw_kms_new_framebuffer_surface(struct
> vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev;
>  	struct vmw_framebuffer_surface *vfbs;
>  	enum SVGA3dSurfaceFormat format;
> +	u32 pixel_format;
>  	int ret;
> 
>  	/* 3D is only supported on HWv8 and newer hosts */
> @@ -548,17 +549,22 @@ static int vmw_kms_new_framebuffer_surface(struct
> vmw_private *dev_priv, return -EINVAL;
>  	}
> 
> +	/* FIXME 'A' format implies per-pixel alpha blending for KMS */
>  	switch (mode_cmd->depth) {
>  	case 32:
> +		pixel_format = DRM_FORMAT_ARGB8888;
>  		format = SVGA3D_A8R8G8B8;
>  		break;
>  	case 24:
> +		pixel_format = DRM_FORMAT_XRGB8888;
>  		format = SVGA3D_X8R8G8B8;
>  		break;
>  	case 16:
> +		pixel_format = DRM_FORMAT_RGB565;
>  		format = SVGA3D_R5G6B5;
>  		break;
>  	case 15:
> +		pixel_format = DRM_FORMAT_ARGB1555;
>  		format = SVGA3D_A1R5G5B5;
>  		break;
>  	default:
> @@ -582,7 +588,8 @@ static int vmw_kms_new_framebuffer_surface(struct
> vmw_private *dev_priv, }
> 
>  	vfbs->base.base.dev = dev;
> -	/* XXX get the first 3 from the surface info */
> +	/* XXX get the first 4 from the surface info */
> +	vfbs->base.base.pixel_format = pixel_format;
>  	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
>  	vfbs->base.base.pitches[0] = mode_cmd->pitch;
>  	vfbs->base.base.depth = mode_cmd->depth;
> @@ -834,6 +841,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev;
>  	struct vmw_framebuffer_dmabuf *vfbd;
>  	unsigned int requested_size;
> +	u32 pixel_format;
>  	int ret;
> 
>  	requested_size = mode_cmd->height * mode_cmd->pitch;
> @@ -852,6 +860,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> vmw_private *dev_priv, if (mode_cmd->bpp == 32)
>  				break;
> 
> +			/* FIXME 'A' format implies per-pixel alpha blending 
for KMS */
> +			if (mode_cmd->depth == 32)
> +				pixel_format = DRM_FORMAT_ARGB8888;
> +			else
> +				pixel_format = DRM_FORMAT_XRGB8888;
> +
>  			DRM_ERROR("Invalid color depth/bbp: %d %d\n",
>  				  mode_cmd->depth, mode_cmd->bpp);
>  			return -EINVAL;
> @@ -861,6 +875,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> vmw_private *dev_priv, if (mode_cmd->bpp == 16)
>  				break;
> 
> +			/* FIXME 'A' format implies per-pixel alpha blending 
for KMS */
> +			if (mode_cmd->depth == 16)
> +				pixel_format = DRM_FORMAT_RGB565;
> +			else
> +				pixel_format = DRM_FORMAT_ARGB1555;
> +
>  			DRM_ERROR("Invalid color depth/bbp: %d %d\n",
>  				  mode_cmd->depth, mode_cmd->bpp);
>  			return -EINVAL;
> @@ -877,6 +897,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> vmw_private *dev_priv, }
> 
>  	vfbd->base.base.dev = dev;
> +	vfbd->base.base.pixel_format = pixel_format;
>  	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
>  	vfbd->base.base.pitches[0] = mode_cmd->pitch;
>  	vfbd->base.base.depth = mode_cmd->depth;
Ville Syrjälä Nov. 17, 2016, 7:27 p.m. UTC | #2
On Thu, Nov 17, 2016 at 07:52:23PM +0200, Laurent Pinchart wrote:
> Hi Ville,
> 
> Thank you for the patch.
> 
> On Thursday 17 Nov 2016 18:14:13 ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Stuff something semi-reasonable into fb->pixel_format. I had to guess
> > as to which formats we should pick. Did I guess correctly?
> > 
> > We can't quite use drm_mode_legacy_fb_format() due to the ARGB1555
> > vs. XRGB155 mess. However use of 'A' formats should imply per-pixel
> > alpha blending as far as KMS is concerned so I'm not at all sure we
> > want to have any 'A' formats exposed as opposed to just 'X' formats.
> > OTOH vmvgfx doesn't do planes, and so the core won't enforce that
> > these formats match any list of supported formats, so the choice
> > shouldn't be super important at this point.
> 
> The vmgfx driver should really be converted to using struct drm_mode_fb_cmd2 
> through the whole code instead of converting struct drm_mode_fb_cmd2 to struct 
> drm_mode_fb_cmd in vmw_kms_fb_create(). I gave it a go a few months back but 
> the structure is passed around so many functions that conversion requires more 
> knowledge about the driver than I have. I don't expect you to fix that, but it 
> would be nice if Sinclair or Thomas could give it a go.

Indeed. Would be nice to bring all drivers into the fold. I didn't
want to start down this particular hole with the series already
too big as is.

> 
> > Cc: linux-graphics-maintainer@vmware.com
> > Cc: Sinclair Yeh <syeh@vmware.com>
> > Cc: Thomas Hellstrom <thellstrom@vmware.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 7d92ab56961b..5788913ca8f9
> > 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > @@ -524,6 +524,7 @@ static int vmw_kms_new_framebuffer_surface(struct
> > vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev;
> >  	struct vmw_framebuffer_surface *vfbs;
> >  	enum SVGA3dSurfaceFormat format;
> > +	u32 pixel_format;
> >  	int ret;
> > 
> >  	/* 3D is only supported on HWv8 and newer hosts */
> > @@ -548,17 +549,22 @@ static int vmw_kms_new_framebuffer_surface(struct
> > vmw_private *dev_priv, return -EINVAL;
> >  	}
> > 
> > +	/* FIXME 'A' format implies per-pixel alpha blending for KMS */
> >  	switch (mode_cmd->depth) {
> >  	case 32:
> > +		pixel_format = DRM_FORMAT_ARGB8888;
> >  		format = SVGA3D_A8R8G8B8;
> >  		break;
> >  	case 24:
> > +		pixel_format = DRM_FORMAT_XRGB8888;
> >  		format = SVGA3D_X8R8G8B8;
> >  		break;
> >  	case 16:
> > +		pixel_format = DRM_FORMAT_RGB565;
> >  		format = SVGA3D_R5G6B5;
> >  		break;
> >  	case 15:
> > +		pixel_format = DRM_FORMAT_ARGB1555;
> >  		format = SVGA3D_A1R5G5B5;
> >  		break;
> >  	default:
> > @@ -582,7 +588,8 @@ static int vmw_kms_new_framebuffer_surface(struct
> > vmw_private *dev_priv, }
> > 
> >  	vfbs->base.base.dev = dev;
> > -	/* XXX get the first 3 from the surface info */
> > +	/* XXX get the first 4 from the surface info */
> > +	vfbs->base.base.pixel_format = pixel_format;
> >  	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
> >  	vfbs->base.base.pitches[0] = mode_cmd->pitch;
> >  	vfbs->base.base.depth = mode_cmd->depth;
> > @@ -834,6 +841,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> > vmw_private *dev_priv, struct drm_device *dev = dev_priv->dev;
> >  	struct vmw_framebuffer_dmabuf *vfbd;
> >  	unsigned int requested_size;
> > +	u32 pixel_format;
> >  	int ret;
> > 
> >  	requested_size = mode_cmd->height * mode_cmd->pitch;
> > @@ -852,6 +860,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> > vmw_private *dev_priv, if (mode_cmd->bpp == 32)
> >  				break;
> > 
> > +			/* FIXME 'A' format implies per-pixel alpha blending 
> for KMS */
> > +			if (mode_cmd->depth == 32)
> > +				pixel_format = DRM_FORMAT_ARGB8888;
> > +			else
> > +				pixel_format = DRM_FORMAT_XRGB8888;
> > +
> >  			DRM_ERROR("Invalid color depth/bbp: %d %d\n",
> >  				  mode_cmd->depth, mode_cmd->bpp);
> >  			return -EINVAL;
> > @@ -861,6 +875,12 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> > vmw_private *dev_priv, if (mode_cmd->bpp == 16)
> >  				break;
> > 
> > +			/* FIXME 'A' format implies per-pixel alpha blending 
> for KMS */
> > +			if (mode_cmd->depth == 16)
> > +				pixel_format = DRM_FORMAT_RGB565;
> > +			else
> > +				pixel_format = DRM_FORMAT_ARGB1555;
> > +
> >  			DRM_ERROR("Invalid color depth/bbp: %d %d\n",
> >  				  mode_cmd->depth, mode_cmd->bpp);
> >  			return -EINVAL;
> > @@ -877,6 +897,7 @@ static int vmw_kms_new_framebuffer_dmabuf(struct
> > vmw_private *dev_priv, }
> > 
> >  	vfbd->base.base.dev = dev;
> > +	vfbd->base.base.pixel_format = pixel_format;
> >  	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
> >  	vfbd->base.base.pitches[0] = mode_cmd->pitch;
> >  	vfbd->base.base.depth = mode_cmd->depth;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 7d92ab56961b..5788913ca8f9 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -524,6 +524,7 @@  static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	struct vmw_framebuffer_surface *vfbs;
 	enum SVGA3dSurfaceFormat format;
+	u32 pixel_format;
 	int ret;
 
 	/* 3D is only supported on HWv8 and newer hosts */
@@ -548,17 +549,22 @@  static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
 		return -EINVAL;
 	}
 
+	/* FIXME 'A' format implies per-pixel alpha blending for KMS */
 	switch (mode_cmd->depth) {
 	case 32:
+		pixel_format = DRM_FORMAT_ARGB8888;
 		format = SVGA3D_A8R8G8B8;
 		break;
 	case 24:
+		pixel_format = DRM_FORMAT_XRGB8888;
 		format = SVGA3D_X8R8G8B8;
 		break;
 	case 16:
+		pixel_format = DRM_FORMAT_RGB565;
 		format = SVGA3D_R5G6B5;
 		break;
 	case 15:
+		pixel_format = DRM_FORMAT_ARGB1555;
 		format = SVGA3D_A1R5G5B5;
 		break;
 	default:
@@ -582,7 +588,8 @@  static int vmw_kms_new_framebuffer_surface(struct vmw_private *dev_priv,
 	}
 
 	vfbs->base.base.dev = dev;
-	/* XXX get the first 3 from the surface info */
+	/* XXX get the first 4 from the surface info */
+	vfbs->base.base.pixel_format = pixel_format;
 	vfbs->base.base.bits_per_pixel = mode_cmd->bpp;
 	vfbs->base.base.pitches[0] = mode_cmd->pitch;
 	vfbs->base.base.depth = mode_cmd->depth;
@@ -834,6 +841,7 @@  static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	struct vmw_framebuffer_dmabuf *vfbd;
 	unsigned int requested_size;
+	u32 pixel_format;
 	int ret;
 
 	requested_size = mode_cmd->height * mode_cmd->pitch;
@@ -852,6 +860,12 @@  static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
 			if (mode_cmd->bpp == 32)
 				break;
 
+			/* FIXME 'A' format implies per-pixel alpha blending for KMS */
+			if (mode_cmd->depth == 32)
+				pixel_format = DRM_FORMAT_ARGB8888;
+			else
+				pixel_format = DRM_FORMAT_XRGB8888;
+
 			DRM_ERROR("Invalid color depth/bbp: %d %d\n",
 				  mode_cmd->depth, mode_cmd->bpp);
 			return -EINVAL;
@@ -861,6 +875,12 @@  static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
 			if (mode_cmd->bpp == 16)
 				break;
 
+			/* FIXME 'A' format implies per-pixel alpha blending for KMS */
+			if (mode_cmd->depth == 16)
+				pixel_format = DRM_FORMAT_RGB565;
+			else
+				pixel_format = DRM_FORMAT_ARGB1555;
+
 			DRM_ERROR("Invalid color depth/bbp: %d %d\n",
 				  mode_cmd->depth, mode_cmd->bpp);
 			return -EINVAL;
@@ -877,6 +897,7 @@  static int vmw_kms_new_framebuffer_dmabuf(struct vmw_private *dev_priv,
 	}
 
 	vfbd->base.base.dev = dev;
+	vfbd->base.base.pixel_format = pixel_format;
 	vfbd->base.base.bits_per_pixel = mode_cmd->bpp;
 	vfbd->base.base.pitches[0] = mode_cmd->pitch;
 	vfbd->base.base.depth = mode_cmd->depth;