* [PATCH] drm/omap: change default signal polarities and drives
@ 2020-04-17 11:41 Tomi Valkeinen
2020-04-17 13:29 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Tomi Valkeinen @ 2020-04-17 11:41 UTC (permalink / raw)
To: dri-devel, Laurent Pinchart; +Cc: Tomi Valkeinen, Jyri Sarha
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);
l = FLD_VAL(onoff, 17, 17) |
FLD_VAL(rf, 16, 16) |
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/omap: change default signal polarities and drives
2020-04-17 11:41 [PATCH] drm/omap: change default signal polarities and drives Tomi Valkeinen
@ 2020-04-17 13:29 ` Laurent Pinchart
2020-04-17 13:34 ` Tomi Valkeinen
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2020-04-17 13:29 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Jyri Sarha, dri-devel
Hi Tomi,
Thank you for the patch.
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,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> l = FLD_VAL(onoff, 17, 17) |
> FLD_VAL(rf, 16, 16) |
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/omap: change default signal polarities and drives
2020-04-17 13:29 ` Laurent Pinchart
@ 2020-04-17 13:34 ` Tomi Valkeinen
2020-04-17 13:39 ` Laurent Pinchart
0 siblings, 1 reply; 4+ messages in thread
From: Tomi Valkeinen @ 2020-04-17 13:34 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Jyri Sarha, dri-devel
On 17/04/2020 16:29, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> 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.
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.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/omap: change default signal polarities and drives
2020-04-17 13:34 ` Tomi Valkeinen
@ 2020-04-17 13:39 ` Laurent Pinchart
0 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2020-04-17 13:39 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Jyri Sarha, dri-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-17 13:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 11:41 [PATCH] drm/omap: change default signal polarities and drives Tomi Valkeinen
2020-04-17 13:29 ` Laurent Pinchart
2020-04-17 13:34 ` Tomi Valkeinen
2020-04-17 13:39 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).