Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

On Wed, Sep 09, 2020 at 05:06:01PM +0100, Kieran Bingham wrote:
> On 09/09/2020 13:08, Ville Syrjälä wrote:
> > On Tue, Sep 08, 2020 at 05:05:48PM +0100, Kieran Bingham wrote:
> >> On 08/09/2020 16:52, Laurent Pinchart wrote:
> >>> On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote:
> >>>> On 06/08/2020 03:26, Laurent Pinchart wrote:
> >>>>> When creating a frame buffer, the driver verifies that the pitches for
> >>>>> the chroma planes match the luma plane. This is done incorrectly for
> >>>>> fully planar YUV formats, without taking horizontal subsampling into
> >>>>> account. Fix it.
> >>>>>
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>>>> ---
> > <snip>
> >>>>>  	}, {
> >>>>>  		.fourcc = DRM_FORMAT_YVU444,
> >>>>>  		.v4l2 = V4L2_PIX_FMT_YVU444M,
> >>>>>  		.bpp = 24,
> >>>>>  		.planes = 3,
> >>>>> +		.hsub = 1,
> >>>>>  	},
> >>>>>  };
> >>>>>  
> >>>>
> >>>> I wonder when we can have a global/generic set of format tables so that
> >>>> all of this isn't duplicated on a per-driver basis.
> >>>
> >>> Note that this table also contains register values, so at least that
> >>> part will need to be kept. For the rest, do you mean a 4CC library that
> >>
> >> Yes, the driver specific mappings of course need to be driver specific.
> >>
> >>> would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad
> >>> it has been shot down when patches were submitted :-S
> >>
> >>  /o\ ... It just seems like so much data replication that must be used
> >> by many drivers.
> >>
> >> Even without mapping the DRM/V4L2 fourccs - even a common table in each
> >> subsystem would be beneficial wouldn't it?
> >>
> >> I mean - RCar-DU isn't the only device that needs to know how many
> >> planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses?
> >>
> >> Anyway, that's not an issue with this patch, it just seems glaring to me
> >> that these entries are common across all hardware that use them ...
> >>
> >> (the bpp/planes/subsampling of course, not the hardware specific registers).
> > 
> > See drm_format_info() & co.
> 
> Aha perfect, That's what I was looking for.
> I'm glad to see that's common (at least for the DRM parts).
> 
> The question is - why aren't we using it in RCar-DU.
> 
> Laurent, would you see any issue in obtaining the struct drm_format_info
> rather than re-encoding all the data in these tables?
> 
> And if not - would you prefer to convert on top of this patch, or
> preceding this patch?
> 
> Or simply prefer to keep the existing tables ?
> 
> Given that this fixes a bug - I'd say getting this patch in now is
> probably best.

I'd apply refactoring on top, if desired. You would have to keep the
existing table for the mapping to V4L2 (although this could be moved to
drm_format_info if desired), as well as for the register value. And that
would then lead to a double lookup operation. That's the part that
bothers me the most.

-- 
Regards,

Laurent Pinchart