Re: Re: [PATCH v5 22/22] drm/sun4i: tv: Convert to the new TV mode property

[Jernej Škrabec]

Dne petek, 14. oktober 2022 ob 09:38:10 CEST je Maxime Ripard napisal(a):
> Hi Jernej,
> 
> On Thu, Oct 13, 2022 at 08:23:51PM +0200, Jernej Škrabec wrote:
> > Dne četrtek, 13. oktober 2022 ob 15:19:06 CEST je Maxime Ripard 
napisal(a):
> > > Now that the core can deal fine with analog TV modes, let's convert the
> > > sun4i TV driver to leverage those new features.
> > > 
> > > Acked-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > > 
> > > ---
> > > Changes in v5:
> > > - Removed the count variable in get_modes
> > > - Removed spurious vc4 change
> > > ---
> > > 
> > >  drivers/gpu/drm/sun4i/sun4i_tv.c | 145
> > > 
> > > +++++++++++++-------------------------- 1 file changed, 48
> > > insertions(+),
> > > 97 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c
> > > b/drivers/gpu/drm/sun4i/sun4i_tv.c index c65f0a89b6b0..4f07aff11551
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
> > > @@ -141,23 +141,14 @@ struct resync_parameters {
> > > 
> > >  struct tv_mode {
> > >  
> > >  	char		*name;
> > > 
> > > +	unsigned int	tv_mode;
> > > +
> > > 
> > >  	u32		mode;
> > >  	u32		chroma_freq;
> > >  	u16		back_porch;
> > >  	u16		front_porch;
> > > 
> > > -	u16		line_number;
> > > 
> > >  	u16		vblank_level;
> > 
> > isn't there a way to get or calculate back_porch, front_porch and
> > vblank_level from mode? From quick glance over removed values below, I
> > would say that at least back_porch can be removed too?
> 
> I tried actually, but I'm not sure what the front porch and back porch
> parameters actually are. They are called that way by Allwinner, but it
> doesn't match the PAL or NTSC timings at all.
> 
> For example, back_porch is 118 for NTSC and 138 for PAL. Actual back
> porches would be around 12 and 16, respectively. Actually, the entire
> blanking area are 138 and 144. This is close enough for PAL, but pretty
> far off for NTSC.
> 
> Allwinner has the habit of integrating the sync period into one of the
> porches, but still there's no obvious match.
> 
> front_porch is pretty much in the same case.

Ok then.

> 
> Since it affected the display output quite a lot, I chose to keep it for
> now unfortunately.
> 
> > Otherwise this patch looks ok.
> 
> Can I add your Acked-by/Reviewed-by then?

Sure.
Reviewed-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>

Best regards,
Jernej