Re: [PATCH v2 12/12] drm/i915: Make sure fb offset is (macro)pixel aligned

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Aug 10, 2016 at 02:33:53PM +0300, Ville Syrjälä wrote:
> On Wed, Aug 10, 2016 at 02:02:43PM +0300, Ville Syrjälä wrote:
> > On Wed, Aug 10, 2016 at 12:04:32PM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 10, 2016 at 12:23:21PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > We convert the fb->offsets[] into x/y offsets, so they must be
> > > > (macro)pixel aligned. Check for this, and if things look good
> > > > allow fb->offsets[] != 0 when creating fbs since we now handle
> > > > them correctly.
> > > > 
> > > > v2: Move to last place in the series and improve the commit message
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v1)
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 37 +++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 34 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index f8487bcdb271..d3de56f0e764 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -15370,6 +15370,37 @@ u32 intel_fb_pitch_limit(struct drm_device *dev, uint64_t fb_modifier,
> > > >  	}
> > > >  }
> > > >  
> > > > +static int intel_fb_check_offsets(const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > +{
> > > > +	uint32_t format = mode_cmd->pixel_format;
> > > > +	int num_planes = drm_format_num_planes(format);
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < num_planes; i++) {
> > > > +		unsigned int cpp;
> > > > +
> > > > +		switch (format) {
> > > > +		case DRM_FORMAT_YUYV:
> > > > +		case DRM_FORMAT_UYVY:
> > > > +		case DRM_FORMAT_YVYU:
> > > > +		case DRM_FORMAT_VYUY:
> > > > +			cpp = 4;
> > > > +			break;
> > > > +		default:
> > > > +			cpp = drm_format_plane_cpp(format, i);
> > > 
> > > I didn't ask for a helper in drm_fourcc.c for macropixel blocksize? Shame
> > > on me!
> > 
> > You did, long ago. I just couldn't be bothered to go and see what we
> > need to do there. Now that I do, it doesn't look like we've gained
> > any new formats that would need special treatment, so I suppose I
> > could just reuse this guy as is.
> 
> Any ideas for the name? Anything I can come up with sounds stupid.
> The _cpp() precedent makes me want to do _cpm(), but that looks
> totally stupid. _macropixel_size() perhaps? Doesn't match the _cpp()
> Maybe rename the _cpp() to _pixel_size() to match? Dunno. Ideas?

I've pushed everything but this last patch to dinq. Thanks for the
reviews.

There are two open questions with this patch: first one is about
moving the helper to the core, and the second one has to do with
tests. Namely, I can't recall where I put the ones I had written,
so I need to try and dig them up.

> 
> > 
> > > Anyway, looks all good and I didn't spot anything else amiss while
> > > re-reading the series.
> > > 
> > > Another wishlist item: Extracting intel_framebuffer.c might be a good
> > > idea.
> > > -Daniel
> > > 
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		if (mode_cmd->offsets[i] % cpp) {
> > > > +			DRM_DEBUG("fb plane %d offset 0x%08x not (macro)pixel aligned\n",
> > > > +				  i, mode_cmd->offsets[i]);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int intel_framebuffer_init(struct drm_device *dev,
> > > >  				  struct intel_framebuffer *intel_fb,
> > > >  				  struct drm_mode_fb_cmd2 *mode_cmd,
> > > > @@ -15514,9 +15545,9 @@ static int intel_framebuffer_init(struct drm_device *dev,
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	/* FIXME need to adjust LINOFF/TILEOFF accordingly. */
> > > > -	if (mode_cmd->offsets[0] != 0)
> > > > -		return -EINVAL;
> > > > +	ret = intel_fb_check_offsets(mode_cmd);
> > > > +	if (ret)
> > > > +		return ret;
> > > >  
> > > >  	drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
 > > >  	intel_fb->obj = obj;
> > > > -- 
> > > > 2.7.4
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx