[PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There will be a need for this function in drm_crtc.c later. This
avoids making drm_crtc.c depend on drm_crtc_helper.c.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c        |   32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_helper.c |   33 ---------------------------------
 include/drm/drm_crtc.h            |    2 ++
 include/drm/drm_crtc_helper.h     |    2 --
 4 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d2d9dc5..a99558e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3454,3 +3454,35 @@ void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 	}
 }
 EXPORT_SYMBOL(drm_fb_get_bpp_depth);
+
+/**
+ * drm_format_num_planes - get the number of planes for format
+ * @format: pixel format (DRM_FORMAT_*)
+ *
+ * RETURNS:
+ * The number of planes used by the specified pixel format.
+ */
+int drm_format_num_planes(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_YUV410:
+	case DRM_FORMAT_YVU410:
+	case DRM_FORMAT_YUV411:
+	case DRM_FORMAT_YVU411:
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_YUV422:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YUV444:
+	case DRM_FORMAT_YVU444:
+		return 3;
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV21:
+	case DRM_FORMAT_NV16:
+	case DRM_FORMAT_NV61:
+		return 2;
+	default:
+		return 1;
+	}
+}
+EXPORT_SYMBOL(drm_format_num_planes);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index d9d6684..dc65e81 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -1017,36 +1017,3 @@ void drm_helper_hpd_irq_event(struct drm_device *dev)
 		queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, 0);
 }
 EXPORT_SYMBOL(drm_helper_hpd_irq_event);
-
-
-/**
- * drm_format_num_planes - get the number of planes for format
- * @format: pixel format (DRM_FORMAT_*)
- *
- * RETURNS:
- * The number of planes used by the specified pixel format.
- */
-int drm_format_num_planes(uint32_t format)
-{
-	switch (format) {
-	case DRM_FORMAT_YUV410:
-	case DRM_FORMAT_YVU410:
-	case DRM_FORMAT_YUV411:
-	case DRM_FORMAT_YVU411:
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-	case DRM_FORMAT_YUV422:
-	case DRM_FORMAT_YVU422:
-	case DRM_FORMAT_YUV444:
-	case DRM_FORMAT_YVU444:
-		return 3;
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV21:
-	case DRM_FORMAT_NV16:
-	case DRM_FORMAT_NV61:
-		return 2;
-	default:
-		return 1;
-	}
-}
-EXPORT_SYMBOL(drm_format_num_planes);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 9595c2c..697c072 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1023,4 +1023,6 @@ extern int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
 
 extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 				 int *bpp);
+extern int drm_format_num_planes(uint32_t format);
+
 #endif /* __DRM_CRTC_H__ */
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 37515d1..3add00e 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -145,6 +145,4 @@ extern void drm_helper_hpd_irq_event(struct drm_device *dev);
 extern void drm_kms_helper_poll_disable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable(struct drm_device *dev);
 
-extern int drm_format_num_planes(uint32_t format);
-
 #endif
-- 
1.7.3.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm: Add drm_format_plane_cpp() utility function

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This function returns the bytes per pixel value based on the pixel
format and plane index.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |    1 +
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a99558e..74979ae 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3486,3 +3486,48 @@ int drm_format_num_planes(uint32_t format)
 	}
 }
 EXPORT_SYMBOL(drm_format_num_planes);
+
+/**
+ * drm_format_plane_cpp - determine the bytes per pixel value
+ * @format: pixel format (DRM_FORMAT_*)
+ * @plane: plane index
+ *
+ * RETURNS:
+ * The bytes per pixel value for the specified plane.
+ */
+int drm_format_plane_cpp(uint32_t format, int plane)
+{
+	unsigned int depth;
+	int bpp;
+
+	if (plane >= drm_format_num_planes(format))
+		return 0;
+
+	switch (format) {
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_YVYU:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+		return 2;
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV21:
+	case DRM_FORMAT_NV16:
+	case DRM_FORMAT_NV61:
+		return plane ? 2 : 1;
+	case DRM_FORMAT_YUV410:
+	case DRM_FORMAT_YVU410:
+	case DRM_FORMAT_YUV411:
+	case DRM_FORMAT_YVU411:
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_YUV422:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YUV444:
+	case DRM_FORMAT_YVU444:
+		return 1;
+	default:
+		drm_fb_get_bpp_depth(format, &depth, &bpp);
+		return bpp >> 3;
+	}
+}
+EXPORT_SYMBOL(drm_format_plane_cpp);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 697c072..ae7be55 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1024,5 +1024,6 @@ extern int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
 extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 				 int *bpp);
 extern int drm_format_num_planes(uint32_t format);
+extern int drm_format_plane_cpp(uint32_t format, int plane);
 
 #endif /* __DRM_CRTC_H__ */
-- 
1.7.3.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm: Add drm_format_{horz, vert}_chroma_subsampling() utility functions

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

These functions return the chroma subsampling factors for the specified
pixel format.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   60 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |    2 +
 2 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 74979ae..95f15c8 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3531,3 +3531,63 @@ int drm_format_plane_cpp(uint32_t format, int plane)
 	}
 }
 EXPORT_SYMBOL(drm_format_plane_cpp);
+
+/**
+ * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
+ * @format: pixel format (DRM_FORMAT_*)
+ *
+ * RETURNS:
+ * The horizontal chroma subsampling factor for the
+ * specified pixel format.
+ */
+int drm_format_horz_chroma_subsampling(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_YUV411:
+	case DRM_FORMAT_YVU411:
+	case DRM_FORMAT_YUV410:
+	case DRM_FORMAT_YVU410:
+		return 4;
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_YVYU:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV21:
+	case DRM_FORMAT_NV16:
+	case DRM_FORMAT_NV61:
+	case DRM_FORMAT_YUV422:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+		return 2;
+	default:
+		return 1;
+	}
+}
+EXPORT_SYMBOL(drm_format_horz_chroma_subsampling);
+
+/**
+ * drm_format_vert_chroma_subsampling - get the vertical chroma subsampling factor
+ * @format: pixel format (DRM_FORMAT_*)
+ *
+ * RETURNS:
+ * The vertical chroma subsampling factor for the
+ * specified pixel format.
+ */
+int drm_format_vert_chroma_subsampling(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_YUV410:
+	case DRM_FORMAT_YVU410:
+		return 4;
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV21:
+		return 2;
+	default:
+		return 1;
+	}
+}
+EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ae7be55..824ab09 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1025,5 +1025,7 @@ extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 				 int *bpp);
 extern int drm_format_num_planes(uint32_t format);
 extern int drm_format_plane_cpp(uint32_t format, int plane);
+extern int drm_format_horz_chroma_subsampling(uint32_t format);
+extern int drm_format_vert_chroma_subsampling(uint32_t format);
 
 #endif /* __DRM_CRTC_H__ */
-- 
1.7.3.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Add drm_format_{horz, vert}_chroma_subsampling() utility functions

[Roland Scheidegger]

Am 05.04.2012 20:35, schrieb ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> These functions return the chroma subsampling factors for the specified
> pixel format.
Hmm not really related but looking at it this reminds me these formats
always look a bit underspecified wrt chroma subsample positions. Are
these fixed, undefined, or what (even mpeg1 and mpeg2 differ there, and
let's not even talk about the big mess that interlaced mpeg2 is)?

Roland



> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |   60 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     |    2 +
>  2 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 74979ae..95f15c8 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3531,3 +3531,63 @@ int drm_format_plane_cpp(uint32_t format, int plane)
>  	}
>  }
>  EXPORT_SYMBOL(drm_format_plane_cpp);
> +
> +/**
> + * drm_format_horz_chroma_subsampling - get the horizontal chroma subsampling factor
> + * @format: pixel format (DRM_FORMAT_*)
> + *
> + * RETURNS:
> + * The horizontal chroma subsampling factor for the
> + * specified pixel format.
> + */
> +int drm_format_horz_chroma_subsampling(uint32_t format)
> +{
> +	switch (format) {
> +	case DRM_FORMAT_YUV411:
> +	case DRM_FORMAT_YVU411:
> +	case DRM_FORMAT_YUV410:
> +	case DRM_FORMAT_YVU410:
> +		return 4;
> +	case DRM_FORMAT_YUYV:
> +	case DRM_FORMAT_YVYU:
> +	case DRM_FORMAT_UYVY:
> +	case DRM_FORMAT_VYUY:
> +	case DRM_FORMAT_NV12:
> +	case DRM_FORMAT_NV21:
> +	case DRM_FORMAT_NV16:
> +	case DRM_FORMAT_NV61:
> +	case DRM_FORMAT_YUV422:
> +	case DRM_FORMAT_YVU422:
> +	case DRM_FORMAT_YUV420:
> +	case DRM_FORMAT_YVU420:
> +		return 2;
> +	default:
> +		return 1;
> +	}
> +}
> +EXPORT_SYMBOL(drm_format_horz_chroma_subsampling);
> +
> +/**
> + * drm_format_vert_chroma_subsampling - get the vertical chroma subsampling factor
> + * @format: pixel format (DRM_FORMAT_*)
> + *
> + * RETURNS:
> + * The vertical chroma subsampling factor for the
> + * specified pixel format.
> + */
> +int drm_format_vert_chroma_subsampling(uint32_t format)
> +{
> +	switch (format) {
> +	case DRM_FORMAT_YUV410:
> +	case DRM_FORMAT_YVU410:
> +		return 4;
> +	case DRM_FORMAT_YUV420:
> +	case DRM_FORMAT_YVU420:
> +	case DRM_FORMAT_NV12:
> +	case DRM_FORMAT_NV21:
> +		return 2;
> +	default:
> +		return 1;
> +	}
> +}
> +EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ae7be55..824ab09 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1025,5 +1025,7 @@ extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
>  				 int *bpp);
>  extern int drm_format_num_planes(uint32_t format);
>  extern int drm_format_plane_cpp(uint32_t format, int plane);
> +extern int drm_format_horz_chroma_subsampling(uint32_t format);
> +extern int drm_format_vert_chroma_subsampling(uint32_t format);
>  
>  #endif /* __DRM_CRTC_H__ */

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Add drm_format_{horz, vert}_chroma_subsampling() utility functions

[Ville Syrjälä]

On Wed, Apr 11, 2012 at 08:53:08PM +0200, Roland Scheidegger wrote:
> Am 05.04.2012 20:35, schrieb ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > These functions return the chroma subsampling factors for the specified
> > pixel format.
> Hmm not really related but looking at it this reminds me these formats
> always look a bit underspecified wrt chroma subsample positions. Are
> these fixed, undefined, or what (even mpeg1 and mpeg2 differ there, and
> let's not even talk about the big mess that interlaced mpeg2 is)?

My thinking is that the sample positions can be configured through
some other means. It doesn't affect the in memory layout at all so
tying it to the format isn't necessary.

This also depends on the hardware. On some hardware you can configure
it quite freely, but on some you may be left with just the one option,
which is often not even documented.

I sent another patch [1] some time ago which provides a helper function
for drivers dealing with good hardware.

[1] http://lists.freedesktop.org/archives/dri-devel/2011-December/017585.html

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/4] drm: Add drm_format_{horz, vert}_chroma_subsampling() utility functions

[Roland Scheidegger]

Am 12.04.2012 14:23, schrieb Ville Syrjälä:
> On Wed, Apr 11, 2012 at 08:53:08PM +0200, Roland Scheidegger wrote:
>> Am 05.04.2012 20:35, schrieb ville.syrjala@linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> These functions return the chroma subsampling factors for the specified
>>> pixel format.
>> Hmm not really related but looking at it this reminds me these formats
>> always look a bit underspecified wrt chroma subsample positions. Are
>> these fixed, undefined, or what (even mpeg1 and mpeg2 differ there, and
>> let's not even talk about the big mess that interlaced mpeg2 is)?
> 
> My thinking is that the sample positions can be configured through
> some other means. It doesn't affect the in memory layout at all so
> tying it to the format isn't necessary.
> 
> This also depends on the hardware. On some hardware you can configure
> it quite freely, but on some you may be left with just the one option,
> which is often not even documented.
> 
> I sent another patch [1] some time ago which provides a helper function
> for drivers dealing with good hardware.
> 
> [1] http://lists.freedesktop.org/archives/dri-devel/2011-December/017585.html
> 

I guess that makes sense. As long as those "other means" are available.
Not like with xv where I think drivers just default to what is probably
the most likely subsample positions, so assuming mpeg2/4 progressive or
something (which gets mpeg1 slightly wrong), if they even bother to care
about such subtleties :-).

Roland

Re: [PATCH 3/4] drm: Add drm_format_{horz, vert}_chroma_subsampling() utility functions

[Ville Syrjälä]

On Fri, Apr 13, 2012 at 04:42:04AM +0200, Roland Scheidegger wrote:
> Am 12.04.2012 14:23, schrieb Ville Syrjälä:
> > On Wed, Apr 11, 2012 at 08:53:08PM +0200, Roland Scheidegger wrote:
> >> Am 05.04.2012 20:35, schrieb ville.syrjala@linux.intel.com:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> These functions return the chroma subsampling factors for the specified
> >>> pixel format.
> >> Hmm not really related but looking at it this reminds me these formats
> >> always look a bit underspecified wrt chroma subsample positions. Are
> >> these fixed, undefined, or what (even mpeg1 and mpeg2 differ there, and
> >> let's not even talk about the big mess that interlaced mpeg2 is)?
> > 
> > My thinking is that the sample positions can be configured through
> > some other means. It doesn't affect the in memory layout at all so
> > tying it to the format isn't necessary.
> > 
> > This also depends on the hardware. On some hardware you can configure
> > it quite freely, but on some you may be left with just the one option,
> > which is often not even documented.
> > 
> > I sent another patch [1] some time ago which provides a helper function
> > for drivers dealing with good hardware.
> > 
> > [1] http://lists.freedesktop.org/archives/dri-devel/2011-December/017585.html
> > 
> 
> I guess that makes sense. As long as those "other means" are available.
> Not like with xv where I think drivers just default to what is probably
> the most likely subsample positions, so assuming mpeg2/4 progressive or
> something (which gets mpeg1 slightly wrong), if they even bother to care
> about such subtleties :-).

Yeah. I think a plane property is the best choice here. If the hardware
doesn't allow you to configure this stuff, the property would just
indicate the fixed configuration that the hardware supports. But this
is again another case where we really want standardised and well
defined properties.

-- 
Ville Syrjälä
Intel OTC

[PATCH 4/4] drm: Add sanity checks to framebuffer creation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Perform some basic sanity check on some of the parameters in
drm_mode_fb_cmd2.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   47 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 95f15c8..e1c8ffb 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2173,6 +2173,47 @@ static int format_check(struct drm_mode_fb_cmd2 *r)
 	}
 }
 
+static int framebuffer_check(struct drm_mode_fb_cmd2 *r)
+{
+	int ret, hsub, vsub, num_planes, i;
+
+	ret = format_check(r);
+	if (ret) {
+		DRM_ERROR("bad framebuffer format 0x%08x\n", r->pixel_format);
+		return ret;
+	}
+
+	hsub = drm_format_horz_chroma_subsampling(r->pixel_format);
+	vsub = drm_format_vert_chroma_subsampling(r->pixel_format);
+	num_planes = drm_format_num_planes(r->pixel_format);
+
+	if (r->width == 0 || r->width % hsub) {
+		DRM_ERROR("bad framebuffer width %u\n", r->height);
+		return -EINVAL;
+	}
+
+	if (r->height == 0 || r->height % vsub) {
+		DRM_ERROR("bad framebuffer height %u\n", r->height);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < num_planes; i++) {
+		unsigned int width = r->width / (i != 0 ? hsub : 1);
+
+		if (!r->handles[i]) {
+			DRM_ERROR("no buffer object handle for plane %d\n", i);
+			return -EINVAL;
+		}
+
+		if (r->pitches[i] < drm_format_plane_cpp(r->pixel_format, i) * width) {
+			DRM_ERROR("bad pitch %u for plane %d\n", r->pitches[i], i);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * drm_mode_addfb2 - add an FB to the graphics configuration
  * @inode: inode from the ioctl
@@ -2212,11 +2253,9 @@ int drm_mode_addfb2(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	ret = format_check(r);
-	if (ret) {
-		DRM_ERROR("bad framebuffer format 0x%08x\n", r->pixel_format);
+	ret = framebuffer_check(r);
+	if (ret)
 		return ret;
-	}
 
 	mutex_lock(&dev->mode_config.mutex);
 
-- 
1.7.3.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[Dave Airlie]

On Thu, Apr 5, 2012 at 7:35 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> There will be a need for this function in drm_crtc.c later. This
> avoids making drm_crtc.c depend on drm_crtc_helper.c.

Hi Ville,

I've applied these 4 patches, but I might have lost some others for
you, let me know if there is some other stuff I should be reviewing
for -next.

Dave.

Re: [PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[Ville Syrjälä]

On Fri, Apr 20, 2012 at 12:43:03PM +0100, Dave Airlie wrote:
> On Thu, Apr 5, 2012 at 7:35 PM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > There will be a need for this function in drm_crtc.c later. This
> > avoids making drm_crtc.c depend on drm_crtc_helper.c.
> 
> Hi Ville,
> 
> I've applied these 4 patches, but I might have lost some others for
> you, let me know if there is some other stuff I should be reviewing
> for -next.

IIRC only one patch fell through the cracks:
Subject: [PATCH] drm: Unify and fix idr error handling
Message-Id: <1331834311-30757-1-git-send-email-ville.syrjala@linux.intel.com>

BTW you never gave any statement wrt. removing NV12M and YUV420M from
drm_fourcc.h. I can send a patch if you agree, and if not, well then
someone has to actually change the code to treat them exactly the same
as NV12 and YUV420.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[Dave Airlie]

On Fri, Apr 20, 2012 at 1:23 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Apr 20, 2012 at 12:43:03PM +0100, Dave Airlie wrote:
>> On Thu, Apr 5, 2012 at 7:35 PM,  <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > There will be a need for this function in drm_crtc.c later. This
>> > avoids making drm_crtc.c depend on drm_crtc_helper.c.
>>
>> Hi Ville,
>>
>> I've applied these 4 patches, but I might have lost some others for
>> you, let me know if there is some other stuff I should be reviewing
>> for -next.
>
> IIRC only one patch fell through the cracks:
> Subject: [PATCH] drm: Unify and fix idr error handling
> Message-Id: <1331834311-30757-1-git-send-email-ville.syrjala@linux.intel.com>

Thanks I'll take a look at that,

>
> BTW you never gave any statement wrt. removing NV12M and YUV420M from
> drm_fourcc.h. I can send a patch if you agree, and if not, well then
> someone has to actually change the code to treat them exactly the same
> as NV12 and YUV420.

I'm probably not qualified, if you and jbarnes and say one other
non-Intel person agree, then send the patch with R-b and I'll apply
it.

Dave.

Re: [PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[Rob Clark]

On Fri, Apr 20, 2012 at 7:29 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 1:23 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Apr 20, 2012 at 12:43:03PM +0100, Dave Airlie wrote:
>>> On Thu, Apr 5, 2012 at 7:35 PM,  <ville.syrjala@linux.intel.com> wrote:
>>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> >
>>> > There will be a need for this function in drm_crtc.c later. This
>>> > avoids making drm_crtc.c depend on drm_crtc_helper.c.
>>>
>>> Hi Ville,
>>>
>>> I've applied these 4 patches, but I might have lost some others for
>>> you, let me know if there is some other stuff I should be reviewing
>>> for -next.
>>
>> IIRC only one patch fell through the cracks:
>> Subject: [PATCH] drm: Unify and fix idr error handling
>> Message-Id: <1331834311-30757-1-git-send-email-ville.syrjala@linux.intel.com>
>
> Thanks I'll take a look at that,
>
>>
>> BTW you never gave any statement wrt. removing NV12M and YUV420M from
>> drm_fourcc.h. I can send a patch if you agree, and if not, well then
>> someone has to actually change the code to treat them exactly the same
>> as NV12 and YUV420.
>
> I'm probably not qualified, if you and jbarnes and say one other
> non-Intel person agree, then send the patch with R-b and I'll apply
> it.

I agree that they seem like the same thing (as their non-M
counterparts) to me..  maybe the one exception is if there was hw that
somehow *only* supported discontiguous multi-planar.  I can't really
tell if this is the case in current exynos, it seems to only advertise
XRGB8888 (but possibly I'm looking in the wrong place).

For drivers that have weird requirements, I'd suggest they use the non
7-bit ascii space (ie. one or more of bytes in fourcc is non-ascii, so
as to not conflict with potential future standard fourcc's) driver
specific format values.

BR,
-R

> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[Ville Syrjälä]

On Fri, Apr 20, 2012 at 07:48:07AM -0500, Rob Clark wrote:
> On Fri, Apr 20, 2012 at 7:29 AM, Dave Airlie <airlied@gmail.com> wrote:
> > On Fri, Apr 20, 2012 at 1:23 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> >> On Fri, Apr 20, 2012 at 12:43:03PM +0100, Dave Airlie wrote:
> >>> On Thu, Apr 5, 2012 at 7:35 PM,  <ville.syrjala@linux.intel.com> wrote:
> >>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> >
> >>> > There will be a need for this function in drm_crtc.c later. This
> >>> > avoids making drm_crtc.c depend on drm_crtc_helper.c.
> >>>
> >>> Hi Ville,
> >>>
> >>> I've applied these 4 patches, but I might have lost some others for
> >>> you, let me know if there is some other stuff I should be reviewing
> >>> for -next.
> >>
> >> IIRC only one patch fell through the cracks:
> >> Subject: [PATCH] drm: Unify and fix idr error handling
> >> Message-Id: <1331834311-30757-1-git-send-email-ville.syrjala@linux.intel.com>
> >
> > Thanks I'll take a look at that,
> >
> >>
> >> BTW you never gave any statement wrt. removing NV12M and YUV420M from
> >> drm_fourcc.h. I can send a patch if you agree, and if not, well then
> >> someone has to actually change the code to treat them exactly the same
> >> as NV12 and YUV420.
> >
> > I'm probably not qualified, if you and jbarnes and say one other
> > non-Intel person agree, then send the patch with R-b and I'll apply
> > it.
> 
> I agree that they seem like the same thing (as their non-M
> counterparts) to me..  maybe the one exception is if there was hw that
> somehow *only* supported discontiguous multi-planar.

The way things are currently, anyone can feed the driver either
contiguous or discontiguous planes using either format.

As a hint to userspace the M version might work, except it still
doesn't tell you anything on how you need to allocate or align the
planes. Since memory allocation is driver specific anyway, I see no
point in overloading the pixel formats with that information. Some
other mechanism to communicate such information would be needed if
there is a desire to make it generic.

> I can't really
> tell if this is the case in current exynos, it seems to only advertise
> XRGB8888 (but possibly I'm looking in the wrong place).
> 
> For drivers that have weird requirements, I'd suggest they use the non
> 7-bit ascii space (ie. one or more of bytes in fourcc is non-ascii, so
> as to not conflict with potential future standard fourcc's) driver
> specific format values.

We could define a DRM_FORMAT_DRIVER_SPECIFIC bit just like we have
DRM_FORMAT_BIG_ENDIAN. We still have three bits to choose from. OTOH
I'm not too worried about standard fourccs since we already differ
from the standard names anyway.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[Rob Clark]

On Fri, Apr 20, 2012 at 8:49 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Apr 20, 2012 at 07:48:07AM -0500, Rob Clark wrote:
>> On Fri, Apr 20, 2012 at 7:29 AM, Dave Airlie <airlied@gmail.com> wrote:
>> > On Fri, Apr 20, 2012 at 1:23 PM, Ville Syrjälä
>> > <ville.syrjala@linux.intel.com> wrote:
>> >> On Fri, Apr 20, 2012 at 12:43:03PM +0100, Dave Airlie wrote:
>> >>> On Thu, Apr 5, 2012 at 7:35 PM,  <ville.syrjala@linux.intel.com> wrote:
>> >>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>> >
>> >>> > There will be a need for this function in drm_crtc.c later. This
>> >>> > avoids making drm_crtc.c depend on drm_crtc_helper.c.
>> >>>
>> >>> Hi Ville,
>> >>>
>> >>> I've applied these 4 patches, but I might have lost some others for
>> >>> you, let me know if there is some other stuff I should be reviewing
>> >>> for -next.
>> >>
>> >> IIRC only one patch fell through the cracks:
>> >> Subject: [PATCH] drm: Unify and fix idr error handling
>> >> Message-Id: <1331834311-30757-1-git-send-email-ville.syrjala@linux.intel.com>
>> >
>> > Thanks I'll take a look at that,
>> >
>> >>
>> >> BTW you never gave any statement wrt. removing NV12M and YUV420M from
>> >> drm_fourcc.h. I can send a patch if you agree, and if not, well then
>> >> someone has to actually change the code to treat them exactly the same
>> >> as NV12 and YUV420.
>> >
>> > I'm probably not qualified, if you and jbarnes and say one other
>> > non-Intel person agree, then send the patch with R-b and I'll apply
>> > it.
>>
>> I agree that they seem like the same thing (as their non-M
>> counterparts) to me..  maybe the one exception is if there was hw that
>> somehow *only* supported discontiguous multi-planar.
>
> The way things are currently, anyone can feed the driver either
> contiguous or discontiguous planes using either format.
>
> As a hint to userspace the M version might work, except it still
> doesn't tell you anything on how you need to allocate or align the
> planes. Since memory allocation is driver specific anyway, I see no
> point in overloading the pixel formats with that information. Some
> other mechanism to communicate such information would be needed if
> there is a desire to make it generic.
>
>> I can't really
>> tell if this is the case in current exynos, it seems to only advertise
>> XRGB8888 (but possibly I'm looking in the wrong place).
>>
>> For drivers that have weird requirements, I'd suggest they use the non
>> 7-bit ascii space (ie. one or more of bytes in fourcc is non-ascii, so
>> as to not conflict with potential future standard fourcc's) driver
>> specific format values.
>
> We could define a DRM_FORMAT_DRIVER_SPECIFIC bit just like we have

yeah, that is a good idea

> DRM_FORMAT_BIG_ENDIAN. We still have three bits to choose from. OTOH
> I'm not too worried about standard fourccs since we already differ
> from the standard names anyway.

well, was more just thinking from the point of view of clashes if we
add more standard names later but some driver somewhere was already
using that new fourcc name

Possibly I'm over-thinking this.. but seemed like a reasonable thing
to separate standard and non-standard formats before a bunch of driver
specific formats start cropping up.

BR,
-R

> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Move drm_format_num_planes() to drm_crtc.c

[InKi Dae]

2012년 4월 20일 오후 11:22, Rob Clark <rob.clark@linaro.org>님의 말:
> On Fri, Apr 20, 2012 at 8:49 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Fri, Apr 20, 2012 at 07:48:07AM -0500, Rob Clark wrote:
>>> On Fri, Apr 20, 2012 at 7:29 AM, Dave Airlie <airlied@gmail.com> wrote:
>>> > On Fri, Apr 20, 2012 at 1:23 PM, Ville Syrjälä
>>> > <ville.syrjala@linux.intel.com> wrote:
>>> >> On Fri, Apr 20, 2012 at 12:43:03PM +0100, Dave Airlie wrote:
>>> >>> On Thu, Apr 5, 2012 at 7:35 PM,  <ville.syrjala@linux.intel.com> wrote:
>>> >>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> >>> >
>>> >>> > There will be a need for this function in drm_crtc.c later. This
>>> >>> > avoids making drm_crtc.c depend on drm_crtc_helper.c.
>>> >>>
>>> >>> Hi Ville,
>>> >>>
>>> >>> I've applied these 4 patches, but I might have lost some others for
>>> >>> you, let me know if there is some other stuff I should be reviewing
>>> >>> for -next.
>>> >>
>>> >> IIRC only one patch fell through the cracks:
>>> >> Subject: [PATCH] drm: Unify and fix idr error handling
>>> >> Message-Id: <1331834311-30757-1-git-send-email-ville.syrjala@linux.intel.com>
>>> >
>>> > Thanks I'll take a look at that,
>>> >
>>> >>
>>> >> BTW you never gave any statement wrt. removing NV12M and YUV420M from
>>> >> drm_fourcc.h. I can send a patch if you agree, and if not, well then
>>> >> someone has to actually change the code to treat them exactly the same
>>> >> as NV12 and YUV420.
>>> >
>>> > I'm probably not qualified, if you and jbarnes and say one other
>>> > non-Intel person agree, then send the patch with R-b and I'll apply
>>> > it.
>>>
>>> I agree that they seem like the same thing (as their non-M
>>> counterparts) to me..  maybe the one exception is if there was hw that
>>> somehow *only* supported discontiguous multi-planar.
>>
>> The way things are currently, anyone can feed the driver either
>> contiguous or discontiguous planes using either format.
>>
>> As a hint to userspace the M version might work, except it still
>> doesn't tell you anything on how you need to allocate or align the
>> planes. Since memory allocation is driver specific anyway, I see no
>> point in overloading the pixel formats with that information. Some
>> other mechanism to communicate such information would be needed if
>> there is a desire to make it generic.
>>
>>> I can't really
>>> tell if this is the case in current exynos, it seems to only advertise
>>> XRGB8888 (but possibly I'm looking in the wrong place).
>>>
>>> For drivers that have weird requirements, I'd suggest they use the non
>>> 7-bit ascii space (ie. one or more of bytes in fourcc is non-ascii, so
>>> as to not conflict with potential future standard fourcc's) driver
>>> specific format values.
>>
>> We could define a DRM_FORMAT_DRIVER_SPECIFIC bit just like we have
>
> yeah, that is a good idea
>
>> DRM_FORMAT_BIG_ENDIAN. We still have three bits to choose from. OTOH
>> I'm not too worried about standard fourccs since we already differ
>> from the standard names anyway.
>
> well, was more just thinking from the point of view of clashes if we
> add more standard names later but some driver somewhere was already
> using that new fourcc name
>
> Possibly I'm over-thinking this.. but seemed like a reasonable thing
> to separate standard and non-standard formats before a bunch of driver
> specific formats start cropping up.
>
> BR,
> -R

we just wanted to use multi-planar format in same way as v4l2 side and
I am not still sure that it's good way to add some codes to identify
them. anyway for now, I'm ok.

Thanks,
Inki Dae.

>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel