Re: [PATCH] drm/omap: change default signal polarities and drives

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

Hi Tomi,

On Fri, Apr 17, 2020 at 04:34:19PM +0300, Tomi Valkeinen wrote:
> On 17/04/2020 16:29, Laurent Pinchart wrote:
> > On Fri, Apr 17, 2020 at 02:41:51PM +0300, Tomi Valkeinen wrote:
> >> If the given videomode does not specify DISPLAY_FLAG_* for the specific
> >> signal property, the driver used a default value. These defaults were
> >> never thought through, as the expectation was that all the DISPLAY_FLAGS
> >> are always set explicitly.
> >>
> >> With DRM bridge and panel drivers this is not the case, and while that
> >> issue should be resolved in the future, it's still good to have sane
> >> signal defaults.
> >>
> >> This patch changes the defaults to what the hardware has as reset
> >> defaults. Also, based on my experience, I think they make sense and are
> >> more likely correct than the defaults without this patch.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>   drivers/gpu/drm/omapdrm/dss/dispc.c | 33 ++++++-----------------------
> >>   1 file changed, 6 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> index dbb90f2d2ccd..6639ee9b05d3 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> @@ -3137,33 +3137,12 @@ static void _dispc_mgr_set_lcd_timings(struct dispc_device *dispc,
> >>   	dispc_write_reg(dispc, DISPC_TIMING_H(channel), timing_h);
> >>   	dispc_write_reg(dispc, DISPC_TIMING_V(channel), timing_v);
> >>   
> >> -	if (vm->flags & DISPLAY_FLAGS_VSYNC_HIGH)
> >> -		vs = false;
> >> -	else
> >> -		vs = true;
> >> -
> >> -	if (vm->flags & DISPLAY_FLAGS_HSYNC_HIGH)
> >> -		hs = false;
> >> -	else
> >> -		hs = true;
> >> -
> >> -	if (vm->flags & DISPLAY_FLAGS_DE_HIGH)
> >> -		de = false;
> >> -	else
> >> -		de = true;
> >> -
> >> -	if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
> >> -		ipc = false;
> >> -	else
> >> -		ipc = true;
> >> -
> >> -	/* always use the 'rf' setting */
> >> -	onoff = true;
> >> -
> >> -	if (vm->flags & DISPLAY_FLAGS_SYNC_POSEDGE)
> >> -		rf = true;
> >> -	else
> >> -		rf = false;
> >> +	vs = !!(vm->flags & DISPLAY_FLAGS_VSYNC_LOW);
> >> +	hs = !!(vm->flags & DISPLAY_FLAGS_HSYNC_LOW);
> >> +	de = !!(vm->flags & DISPLAY_FLAGS_DE_LOW);
> >> +	ipc = !!(vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE);
> >> +	onoff = true; /* always use the 'rf' setting */
> >> +	rf = !!(vm->flags & DISPLAY_FLAGS_SYNC_POSEDGE);
> > 
> > Would it make sense to WARN() if flags are not set, to catch offenders
> > and fix them ? Apart from that,
> 
> Maybe at some point, but for now it would probably be printing WARNs on all boards. I haven't looked 
> at exactly which driver combinations get the bus flags/formats right, but I have a feeling that it's 
> not too many.

Fair enough. Maybe we should try it locally first and see how many
components are faulty.

> And some pieces of hardware also may be "don't care" for certain flags, so I think it's a valid case 
> that a bridge/panel doesn't define some of the flags.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel