Re: [PATCH v4 2/3] drm/vc4: plane: Add support for DRM_FORMAT_P030

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Thomas,

Thanks for your review

On Wed, Dec 15, 2021 at 04:11:50PM +0100, Thomas Zimmermann wrote:
> Hi,
> 
> I have a number of comments below. But if you want, you can add
> 
> Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Thanks :)

> Am 15.12.21 um 10:17 schrieb Maxime Ripard:
> > From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > 
> > The P030 format, used with the DRM_FORMAT_MOD_BROADCOM_SAND128 modifier,
> > is a format output by the video decoder on the BCM2711.
> > 
> > Add native support to the KMS planes for that format.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/vc4/vc4_plane.c | 127 ++++++++++++++++++++++++--------
> >   1 file changed, 96 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index ac761c683663..022cd12f561e 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -33,6 +33,7 @@ static const struct hvs_format {
> >   	u32 hvs; /* HVS_FORMAT_* */
> >   	u32 pixel_order;
> >   	u32 pixel_order_hvs5;
> > +	bool hvs5_only;
> >   } hvs_formats[] = {
> >   	{
> >   		.drm = DRM_FORMAT_XRGB8888,
> > @@ -128,6 +129,12 @@ static const struct hvs_format {
> >   		.hvs = HVS_PIXEL_FORMAT_YCBCR_YUV422_2PLANE,
> >   		.pixel_order = HVS_PIXEL_ORDER_XYCRCB,
> >   	},
> > +	{
> > +		.drm = DRM_FORMAT_P030,
> > +		.hvs = HVS_PIXEL_FORMAT_YCBCR_10BIT,
> > +		.pixel_order = HVS_PIXEL_ORDER_XYCBCR,
> > +		.hvs5_only = true,
> > +	},
> >   };
> >   static const struct hvs_format *vc4_get_hvs_format(u32 drm_format)
> > @@ -762,47 +769,90 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
> >   	case DRM_FORMAT_MOD_BROADCOM_SAND128:
> >   	case DRM_FORMAT_MOD_BROADCOM_SAND256: {
> >   		uint32_t param = fourcc_mod_broadcom_param(fb->modifier);
> > -		u32 tile_w, tile, x_off, pix_per_tile;
> > -
> > -		hvs_format = HVS_PIXEL_FORMAT_H264;
> > -
> > -		switch (base_format_mod) {
> > -		case DRM_FORMAT_MOD_BROADCOM_SAND64:
> > -			tiling = SCALER_CTL0_TILING_64B;
> > -			tile_w = 64;
> > -			break;
> > -		case DRM_FORMAT_MOD_BROADCOM_SAND128:
> > -			tiling = SCALER_CTL0_TILING_128B;
> > -			tile_w = 128;
> > -			break;
> > -		case DRM_FORMAT_MOD_BROADCOM_SAND256:
> > -			tiling = SCALER_CTL0_TILING_256B_OR_T;
> > -			tile_w = 256;
> > -			break;
> > -		default:
> > -			break;
> > -		}
> >   		if (param > SCALER_TILE_HEIGHT_MASK) {
> > -			DRM_DEBUG_KMS("SAND height too large (%d)\n", param);
> > +			DRM_DEBUG_KMS("SAND height too large (%d)\n",
> > +				      param);
> 
> Should be good for the 100-character limit.
> 
> >   			return -EINVAL;
> >   		}
> > -		pix_per_tile = tile_w / fb->format->cpp[0];
> > -		tile = vc4_state->src_x / pix_per_tile;
> > -		x_off = vc4_state->src_x % pix_per_tile;
> > +		if (fb->format->format == DRM_FORMAT_P030) {
> > +			hvs_format = HVS_PIXEL_FORMAT_YCBCR_10BIT;
> > +			tiling = SCALER_CTL0_TILING_128B;
> > +		} else {
> > +			hvs_format = HVS_PIXEL_FORMAT_H264;
> > +
> > +			switch (base_format_mod) {
> > +			case DRM_FORMAT_MOD_BROADCOM_SAND64:
> > +				tiling = SCALER_CTL0_TILING_64B;
> > +				break;
> > +			case DRM_FORMAT_MOD_BROADCOM_SAND128:
> > +				tiling = SCALER_CTL0_TILING_128B;
> > +				break;
> > +			case DRM_FORMAT_MOD_BROADCOM_SAND256:
> > +				tiling = SCALER_CTL0_TILING_256B_OR_T;
> > +				break;
> > +			default:
> > +				return -EINVAL;
> 
> It's not atomic modesetting? I'm asking because the code returns errno codes
> in several places.

This is atomic modesetting, but we're allowed to return an error at this
point :)

The function name is a bit confusing but it's mostly due to how the
hardware operates. We don't have the usual register set for the
composition but instead we have a list of hardware descriptors that will
describe the next frame.

The driver builds it here, at atomic_check time, and then copy it to the
hardware at atomic_commit time. So even though it's called
vc4_plane_mode_set, and does the operations needed to setup the
composition properly, we're still in atomic_check at this point.

> > +			}
> > +		}
> >   		/* Adjust the base pointer to the first pixel to be scanned
> >   		 * out.
> > +		 *
> > +		 * For P030, y_ptr [31:4] is the 128bit word for the start pixel
> > +		 * y_ptr [3:0] is the pixel (0-11) contained within that 128bit
> > +		 * word that should be taken as the first pixel.
> > +		 * Ditto uv_ptr [31:4] vs [3:0], however [3:0] contains the
> > +		 * element within the 128bit word, eg for pixel 3 the value
> > +		 * should be 6.
> >   		 */
> >   		for (i = 0; i < num_planes; i++) {
> > +			u32 tile_w, tile, x_off, pix_per_tile;
> > +
> > +			if (fb->format->format == DRM_FORMAT_P030) {
> > +				/*
> > +				 * Spec says: bits [31:4] of the given address
> > +				 * should point to the 128-bit word containing
> > +				 * the desired starting pixel, and bits[3:0]
> > +				 * should be between 0 and 11, indicating which
> > +				 * of the 12-pixels in that 128-bit word is the
> > +				 * first pixel to be used
> > +				 */
> > +				u32 remaining_pixels = vc4_state->src_x % 96;
> > +				u32 aligned = remaining_pixels / 12;
> > +				u32 last_bits = remaining_pixels % 12;
> > +
> > +				x_off = aligned * 16 + last_bits;
> > +				tile_w = 128;
> > +				pix_per_tile = 96;
> > +			} else {
> > +				switch (base_format_mod) {
> > +				case DRM_FORMAT_MOD_BROADCOM_SAND64:
> > +					tile_w = 64;
> > +					break;
> > +				case DRM_FORMAT_MOD_BROADCOM_SAND128:
> > +					tile_w = 128;
> > +					break;
> > +				case DRM_FORMAT_MOD_BROADCOM_SAND256:
> > +					tile_w = 256;
> > +					break;
> > +				default:
> > +					return -EINVAL;
> > +				}
> > +				pix_per_tile = tile_w / fb->format->cpp[0];
> > +				x_off = (vc4_state->src_x % pix_per_tile) /
> > +					(i ? h_subsample : 1) *
> > +					fb->format->cpp[i];
> > +			}
> > +
> > +			tile = vc4_state->src_x / pix_per_tile;
> > +
> 
> It's hard to read. If you can put some of these switches into helpers, it
> might be worth it.

Indeed. The whole function would need some though, is it ok if I send a
subsequent patch to fix it after merging this one?

> >   			vc4_state->offsets[i] += param * tile_w * tile;
> >   			vc4_state->offsets[i] += src_y /
> >   						 (i ? v_subsample : 1) *
> >   						 tile_w;
> > -			vc4_state->offsets[i] += x_off /
> > -						 (i ? h_subsample : 1) *
> > -						 fb->format->cpp[i];
> > +			vc4_state->offsets[i] += x_off & ~(i ? 1 : 0);
> >   		}
> >   		pitch0 = VC4_SET_FIELD(param, SCALER_TILE_HEIGHT);
> > @@ -955,7 +1005,8 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
> >   	/* Pitch word 1/2 */
> >   	for (i = 1; i < num_planes; i++) {
> > -		if (hvs_format != HVS_PIXEL_FORMAT_H264) {
> > +		if (hvs_format != HVS_PIXEL_FORMAT_H264 &&
> > +		    hvs_format != HVS_PIXEL_FORMAT_YCBCR_10BIT) {
> 
> This branch's condition looks like is could be a little helper.
> 
> >   			vc4_dlist_write(vc4_state,
> >   					VC4_SET_FIELD(fb->pitches[i],
> >   						      SCALER_SRC_PITCH));
> > @@ -1315,6 +1366,13 @@ static bool vc4_format_mod_supported(struct drm_plane *plane,
> >   		default:
> >   			return false;
> >   		}
> > +	case DRM_FORMAT_P030:
> > +		switch (fourcc_mod_broadcom_mod(modifier)) {
> > +		case DRM_FORMAT_MOD_BROADCOM_SAND128:
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> >   	case DRM_FORMAT_RGBX1010102:
> >   	case DRM_FORMAT_BGRX1010102:
> >   	case DRM_FORMAT_RGBA1010102:
> > @@ -1347,8 +1405,11 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
> >   	struct drm_plane *plane = NULL;
> >   	struct vc4_plane *vc4_plane;
> >   	u32 formats[ARRAY_SIZE(hvs_formats)];
> > +	int num_formats = 0;
> >   	int ret = 0;
> >   	unsigned i;
> > +	bool hvs5 = of_device_is_compatible(dev->dev->of_node,
> > +					    "brcm,bcm2711-vc5");
> 
> Maybe also a little helper function?
> 
> >   	static const uint64_t modifiers[] = {
> >   		DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED,
> >   		DRM_FORMAT_MOD_BROADCOM_SAND128,
> > @@ -1363,13 +1424,17 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
> >   	if (!vc4_plane)
> >   		return ERR_PTR(-ENOMEM);
> > -	for (i = 0; i < ARRAY_SIZE(hvs_formats); i++)
> > -		formats[i] = hvs_formats[i].drm;
> > +	for (i = 0; i < ARRAY_SIZE(hvs_formats); i++) {
> > +		if (!hvs_formats[i].hvs5_only || hvs5) {
> > +			formats[num_formats] = hvs_formats[i].drm;
> > +			num_formats++;
> > +		}
> > +	}
> 
> With this loop, could num_formats ever be 0?

It shouldn't no, unless the older generation didn't define any format,
which is highly unlikely.

Maxime

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux