diff mbox series

[3/3] drm/mipi: use dcs write for mipi_dsi_dcs_set_tear_scanline

Message ID 20200505160329.2976059-3-emil.l.velikov@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/dsi: use stack buffer in mipi_dsi_dcs_write() | expand

Commit Message

Emil Velikov May 5, 2020, 4:03 p.m. UTC 
From: Emil Velikov <emil.velikov@collabora.com>

The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently
using the generic write. This does not look right.

Perhaps some platforms don't distinguish between the two writers?

Cc: Robert Chiras <robert.chiras@nxp.com>
Cc: Vinay Simha BN <simhavcs@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Thierry Reding <treding@nvidia.com>
Fixes: e83950816367 ("drm/dsi: Implement set tear scanline")
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Robert, can you please test this against the only user - the Raydium
RM67191 panel driver that you introduced.

Thanks

Vinay, can you confirm if this is a genuine typo or there's something
really subtle happening.
---
 drivers/gpu/drm/drm_mipi_dsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Vinay Simha B N May 7, 2020, 12:29 p.m. UTC | #1 
Emil,

Reply inline

On Tue, 5 May 2020 at 9:35 PM, Emil Velikov <emil.l.velikov@gmail.com>
wrote:

> From: Emil Velikov <emil.velikov@collabora.com>
>
> The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently
> using the generic write. This does not look right.
>
> Perhaps some platforms don't distinguish between the two writers?
>
> Cc: Robert Chiras <robert.chiras@nxp.com>
> Cc: Vinay Simha BN <simhavcs@gmail.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Fixes: e83950816367 ("drm/dsi: Implement set tear scanline")
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Robert, can you please test this against the only user - the Raydium
> RM67191 panel driver that you introduced.
>
> Thanks
>
> Vinay, can you confirm if this is a genuine typo or there's something
> really subtle happening.

this has been tested on nexus 7 with jdi panel. I did not understand what
is the typo here?
We need to use DC’s write instead of generic write?

>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c
> b/drivers/gpu/drm/drm_mipi_dsi.c
> index b96d5b4629d7..07102d8da58f 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -1082,11 +1082,11 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
>   */
>  int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16
> scanline)
>  {
> -       u8 payload[3] = { MIPI_DCS_SET_TEAR_SCANLINE, scanline >> 8,
> -                         scanline & 0xff };
> +       u8 payload[2] = { scanline >> 8, scanline & 0xff };
>         ssize_t err;
>
> -       err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
> +       err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE, payload,
> +                                sizeof(payload));
>         if (err < 0)
>                 return err;
>
> --
> 2.25.1
>
> --
regards,
vinaysimha
Emil Velikov May 7, 2020, 4:18 p.m. UTC | #2 
On Thu, 7 May 2020 at 13:29, Vinay Simha B N <simhavcs@gmail.com> wrote:
>
> Emil,
>
> Reply inline
>
> On Tue, 5 May 2020 at 9:35 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently
>> using the generic write. This does not look right.
>>
>> Perhaps some platforms don't distinguish between the two writers?
>>
>> Cc: Robert Chiras <robert.chiras@nxp.com>
>> Cc: Vinay Simha BN <simhavcs@gmail.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Thierry Reding <treding@nvidia.com>
>> Fixes: e83950816367 ("drm/dsi: Implement set tear scanline")
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>> Robert, can you please test this against the only user - the Raydium
>> RM67191 panel driver that you introduced.
>>
>> Thanks
>>
>> Vinay, can you confirm if this is a genuine typo or there's something
>> really subtle happening.
>
> this has been tested on nexus 7 with jdi panel.
The jdi panel (JDI LT070ME05000 I believe) does not use the function, hmm.

Looking through the ML archive - the call in the first 4 revisions of the patch.
Then with v5 it has magically disappeared alongside mipi_dsi_dcs_set_tear_on().

No comment explaining why though - does the driver work w/o both of those?

> I did not understand what is the typo here?
> We need to use DC’s write instead of generic write?

I believe the clue is in the command name - MIPI_DSI_DCS. I was going
to double-check with the spec although it's members only :-\
Based on the usage in DRM, all DCS commands are issued via
mipi_dsi_dcs_{read,write}

Thanks
Emil
Emil Velikov May 13, 2020, 9:44 a.m. UTC | #3 
Hi Vinay,

On Thu, 7 May 2020 at 17:18, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Thu, 7 May 2020 at 13:29, Vinay Simha B N <simhavcs@gmail.com> wrote:
> >
> > Emil,
> >
> > Reply inline
> >
> > On Tue, 5 May 2020 at 9:35 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>
> >> From: Emil Velikov <emil.velikov@collabora.com>
> >>
> >> The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently
> >> using the generic write. This does not look right.
> >>
> >> Perhaps some platforms don't distinguish between the two writers?
> >>
> >> Cc: Robert Chiras <robert.chiras@nxp.com>
> >> Cc: Vinay Simha BN <simhavcs@gmail.com>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >> Cc: Thierry Reding <treding@nvidia.com>
> >> Fixes: e83950816367 ("drm/dsi: Implement set tear scanline")
> >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >> ---
> >> Robert, can you please test this against the only user - the Raydium
> >> RM67191 panel driver that you introduced.
> >>
> >> Thanks
> >>
> >> Vinay, can you confirm if this is a genuine typo or there's something
> >> really subtle happening.
> >
> > this has been tested on nexus 7 with jdi panel.
> The jdi panel (JDI LT070ME05000 I believe) does not use the function, hmm.
>
> Looking through the ML archive - the call in the first 4 revisions of the patch.
> Then with v5 it has magically disappeared alongside mipi_dsi_dcs_set_tear_on().
>
> No comment explaining why though - does the driver work w/o both of those?
>
Any ideas, does the driver work in today's state?

> > I did not understand what is the typo here?
> > We need to use DC’s write instead of generic write?
>
> I believe the clue is in the command name - MIPI_DSI_DCS. I was going
> to double-check with the spec although it's members only :-\
> Based on the usage in DRM, all DCS commands are issued via
> mipi_dsi_dcs_{read,write}
>
Do you agree with the rationale? Alternatively, do you have a
reference to the Android tree where the generic write is used.

Thanks
Emil
Thierry Reding June 5, 2020, 5:36 p.m. UTC | #4 
On Tue, May 05, 2020 at 05:03:29PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently
> using the generic write. This does not look right.
> 
> Perhaps some platforms don't distinguish between the two writers?

I don't think platforms usually care about this level of detail. The
Tegra driver for example doesn't really look at the packet type and just
packets the data in the standard DSI format and then sends it off on the
bus. It does inspect some fields of the packet, but none that are
related to the packet type, I think.

So it's possible that the panel will accept the packet irrespective of
type and handle it correctly. I can imagine that the decoding logic in
these panels might be rather primitive, so perhaps it's not very strict
as to what exactly the type is as long as it can do something with the
data.

In any case, it does make sense to send DCS commands using the DCS type,
so I'd say let's merge this and see if somebody complains:

Reviewed-by: Thierry Reding <treding@nvidia.com>

> Cc: Robert Chiras <robert.chiras@nxp.com>
> Cc: Vinay Simha BN <simhavcs@gmail.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Fixes: e83950816367 ("drm/dsi: Implement set tear scanline")
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Robert, can you please test this against the only user - the Raydium
> RM67191 panel driver that you introduced.
> 
> Thanks
> 
> Vinay, can you confirm if this is a genuine typo or there's something
> really subtle happening.
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index b96d5b4629d7..07102d8da58f 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -1082,11 +1082,11 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
>   */
>  int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline)
>  {
> -	u8 payload[3] = { MIPI_DCS_SET_TEAR_SCANLINE, scanline >> 8,
> -			  scanline & 0xff };
> +	u8 payload[2] = { scanline >> 8, scanline & 0xff };
>  	ssize_t err;
>  
> -	err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
> +	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE, payload,
> +				 sizeof(payload));
>  	if (err < 0)
>  		return err;
>  
> -- 
> 2.25.1
>
Vinay Simha B N June 7, 2020, 4:17 p.m. UTC | #5 
hi emil,

On Wed, May 13, 2020 at 3:17 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Vinay,
>
> On Thu, 7 May 2020 at 17:18, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > On Thu, 7 May 2020 at 13:29, Vinay Simha B N <simhavcs@gmail.com> wrote:
> > >
> > > Emil,
> > >
> > > Reply inline
> > >
> > > On Tue, 5 May 2020 at 9:35 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >>
> > >> From: Emil Velikov <emil.velikov@collabora.com>
> > >>
> > >> The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently
> > >> using the generic write. This does not look right.
> > >>
> > >> Perhaps some platforms don't distinguish between the two writers?
> > >>
> > >> Cc: Robert Chiras <robert.chiras@nxp.com>
> > >> Cc: Vinay Simha BN <simhavcs@gmail.com>
> > >> Cc: Jani Nikula <jani.nikula@intel.com>
> > >> Cc: Thierry Reding <treding@nvidia.com>
> > >> Fixes: e83950816367 ("drm/dsi: Implement set tear scanline")
> > >> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > >> ---
> > >> Robert, can you please test this against the only user - the Raydium
> > >> RM67191 panel driver that you introduced.
> > >>
> > >> Thanks
> > >>
> > >> Vinay, can you confirm if this is a genuine typo or there's something
> > >> really subtle happening.
> > >
> > > this has been tested on nexus 7 with jdi panel.
> > The jdi panel (JDI LT070ME05000 I believe) does not use the function, hmm.
> >
> > Looking through the ML archive - the call in the first 4 revisions of the patch.
> > Then with v5 it has magically disappeared alongside mipi_dsi_dcs_set_tear_on().
> >
> > No comment explaining why though - does the driver work w/o both of those?
> >
> Any ideas, does the driver work in today's state?
Initially I had used cmd mode, later modified to video mode panel,
since to control the panel in cmd mode is not available for mdp4.
so mipi_dsi_dcs_set_tear_on was not used.


>
> > > I did not understand what is the typo here?
> > > We need to use DC’s write instead of generic write?
> >
> > I believe the clue is in the command name - MIPI_DSI_DCS. I was going
> > to double-check with the spec although it's members only :-\
> > Based on the usage in DRM, all DCS commands are issued via
> > mipi_dsi_dcs_{read,write}
> >
> Do you agree with the rationale? Alternatively, do you have a
> reference to the Android tree where the generic write is used.
>
default android nexus 7 kernel
https://github.com/vinaysimhabn/msm/commits/nexus7-msm-flo-3.4-lollipop-release
> Thanks
> Emil



--
regards,
vinaysimha
Sam Ravnborg June 29, 2020, 7:47 a.m. UTC | #6 
On Tue, May 05, 2020 at 05:03:29PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> The helper uses the MIPI_DCS_SET_TEAR_SCANLINE, although it's currently
> using the generic write. This does not look right.
> 
> Perhaps some platforms don't distinguish between the two writers?
> 
> Cc: Robert Chiras <robert.chiras@nxp.com>
> Cc: Vinay Simha BN <simhavcs@gmail.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Fixes: e83950816367 ("drm/dsi: Implement set tear scanline")
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Robert, can you please test this against the only user - the Raydium
> RM67191 panel driver that you introduced.
> 
> Thanks
> 
> Vinay, can you confirm if this is a genuine typo or there's something
> really subtle happening.
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

The discussion did not reveal anything new. Followed the advice of
Thierry and applied.

	Sam

> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index b96d5b4629d7..07102d8da58f 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -1082,11 +1082,11 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
>   */
>  int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline)
>  {
> -	u8 payload[3] = { MIPI_DCS_SET_TEAR_SCANLINE, scanline >> 8,
> -			  scanline & 0xff };
> +	u8 payload[2] = { scanline >> 8, scanline & 0xff };
>  	ssize_t err;
>  
> -	err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
> +	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE, payload,
> +				 sizeof(payload));
>  	if (err < 0)
>  		return err;
>  
> -- 
> 2.25.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index b96d5b4629d7..07102d8da58f 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -1082,11 +1082,11 @@  EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
  */
 int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline)
 {
-	u8 payload[3] = { MIPI_DCS_SET_TEAR_SCANLINE, scanline >> 8,
-			  scanline & 0xff };
+	u8 payload[2] = { scanline >> 8, scanline & 0xff };
 	ssize_t err;
 
-	err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE, payload,
+				 sizeof(payload));
 	if (err < 0)
 		return err;