All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: javierm@redhat.com, dri-devel@lists.freedesktop.org,
	virtualization@lists.linux-foundation.org, airlied@redhat.com,
	sam@ravnborg.org
Subject: Re: [PATCH 17/17] drm/cirrus: Use VGA macro constants to unblank
Date: Thu, 16 Feb 2023 19:20:56 +0200	[thread overview]
Message-ID: <Y+5l+BbN7JjpaQlH@intel.com> (raw)
In-Reply-To: <ba4daf50-4882-5009-5c68-4385cacfdccb@suse.de>

On Thu, Feb 16, 2023 at 02:21:43PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 16.02.23 um 13:52 schrieb Ville Syrjälä:
> > On Thu, Feb 16, 2023 at 01:03:02PM +0100, Thomas Zimmermann wrote:
> >> Hi,
> >>
> >> thanks for taking a look at the patches.
> >>
> >> Am 16.02.23 um 12:33 schrieb Gerd Hoffmann:
> >>> On Wed, Feb 15, 2023 at 05:15:17PM +0100, Thomas Zimmermann wrote:
> >>>> Set the VGA bit for unblanking with macro constants instead of magic
> >>>> values. No functional changes.
> >>>
> >>> blank/unblank should work simliar to bochs (see commit 250e743915d4),
> >>> that is maybe a nice thing to add of you modernize the driver anyway.
> >> Yeah, it's the VGA PAS field. [1] But is it really called blanking? PAS
> >> controls palette access, but blanking is sounds more like DPMS.
> > 
> > Why aren't people just using the normal way of flipping the
> > screen off bit in sequencer register 01?
> 
> Setting the SD bit in SR01 isn't a bad idea. We can do this as part of 
> enabling/disabling the plane.
> 
> But for PAS, we don't have a choice. It's one of the bazillion obscure 
> VGA settings and (according to a comment in the source code) we need to 
> update it for compatibility.

Well, you do need to enable the palette to see something
other that border color. Not sure tha't a very obscure thing :P

On a related note, the code looks pretty sketchy. It just
blindly writes to 0x3c0 assuming it is the attribute controller
index register. But unless you explicitly reset the flip-flop
it could actually be the data write register instead. That could
easily happen if the previous access to the attribute controller
was a read since reads do not toggle the register role.

-- 
Ville Syrjälä
Intel
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

WARNING: multiple messages have this Message-ID (diff)
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: javierm@redhat.com, dri-devel@lists.freedesktop.org,
	virtualization@lists.linux-foundation.org,
	Gerd Hoffmann <kraxel@redhat.com>,
	airlied@redhat.com, sam@ravnborg.org
Subject: Re: [PATCH 17/17] drm/cirrus: Use VGA macro constants to unblank
Date: Thu, 16 Feb 2023 19:20:56 +0200	[thread overview]
Message-ID: <Y+5l+BbN7JjpaQlH@intel.com> (raw)
In-Reply-To: <ba4daf50-4882-5009-5c68-4385cacfdccb@suse.de>

On Thu, Feb 16, 2023 at 02:21:43PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 16.02.23 um 13:52 schrieb Ville Syrjälä:
> > On Thu, Feb 16, 2023 at 01:03:02PM +0100, Thomas Zimmermann wrote:
> >> Hi,
> >>
> >> thanks for taking a look at the patches.
> >>
> >> Am 16.02.23 um 12:33 schrieb Gerd Hoffmann:
> >>> On Wed, Feb 15, 2023 at 05:15:17PM +0100, Thomas Zimmermann wrote:
> >>>> Set the VGA bit for unblanking with macro constants instead of magic
> >>>> values. No functional changes.
> >>>
> >>> blank/unblank should work simliar to bochs (see commit 250e743915d4),
> >>> that is maybe a nice thing to add of you modernize the driver anyway.
> >> Yeah, it's the VGA PAS field. [1] But is it really called blanking? PAS
> >> controls palette access, but blanking is sounds more like DPMS.
> > 
> > Why aren't people just using the normal way of flipping the
> > screen off bit in sequencer register 01?
> 
> Setting the SD bit in SR01 isn't a bad idea. We can do this as part of 
> enabling/disabling the plane.
> 
> But for PAS, we don't have a choice. It's one of the bazillion obscure 
> VGA settings and (according to a comment in the source code) we need to 
> update it for compatibility.

Well, you do need to enable the palette to see something
other that border color. Not sure tha't a very obscure thing :P

On a related note, the code looks pretty sketchy. It just
blindly writes to 0x3c0 assuming it is the attribute controller
index register. But unless you explicitly reset the flip-flop
it could actually be the data write register instead. That could
easily happen if the previous access to the attribute controller
was a read since reads do not toggle the register role.

-- 
Ville Syrjälä
Intel

  reply	other threads:[~2023-02-16 17:21 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-15 16:15 [PATCH 00/17] cirrus: Modernize the cirrus driver Thomas Zimmermann
2023-02-15 16:15 ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 01/17] drm/cirrus: Compute blit destination offset in single location Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 02/17] drm/cirrus: Replace cpp value with format Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 03/17] drm/cirrus: Use drm_fb_blit() to update scanout buffer Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 04/17] drm/cirrus: Move drm_dev_{enter, exit}() into DRM helpers Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 05/17] drm/cirrus: Split cirrus_mode_set() into smaller functions Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 06/17] drm/cirrus: Integrate connector into pipeline code Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 07/17] drm/cirrus: Move primary-plane format arrays Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 08/17] drm/cirrus: Convert to regular atomic helpers Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 09/17] drm/cirrus: Enable damage clipping on primary plane Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 10/17] drm/cirrus: Inline cirrus_fb_blit_rect() Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 11/17] drm/cirrus: Remove format test from cirrus_fb_create() Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 12/17] drm/cirrus: Remove size " Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 13/17] drm/cirrus: Test mode against video-memory size in device-wide mode_valid Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 14/17] drm/cirrus: Inline cirrus_check_size() into primary-plane atomic_check Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 15/17] drm/cirrus: Introduce struct cirrus_primary_plane_state Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 16/17] drm/cirrus: Store HW format/pitch in primary-plane state Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-15 16:15 ` [PATCH 17/17] drm/cirrus: Use VGA macro constants to unblank Thomas Zimmermann
2023-02-15 16:15   ` Thomas Zimmermann
2023-02-16 11:33   ` Gerd Hoffmann
2023-02-16 11:33     ` Gerd Hoffmann
2023-02-16 12:03     ` Thomas Zimmermann
2023-02-16 12:03       ` Thomas Zimmermann
2023-02-16 12:33       ` Gerd Hoffmann
2023-02-16 12:33         ` Gerd Hoffmann
2023-02-16 12:52       ` Ville Syrjälä
2023-02-16 12:52         ` Ville Syrjälä
2023-02-16 13:19         ` Gerd Hoffmann
2023-02-16 13:19           ` Gerd Hoffmann
2023-02-16 13:21         ` Thomas Zimmermann
2023-02-16 13:21           ` Thomas Zimmermann
2023-02-16 17:20           ` Ville Syrjälä [this message]
2023-02-16 17:20             ` Ville Syrjälä
2023-02-17  8:23             ` Thomas Zimmermann
2023-02-17  8:23               ` Thomas Zimmermann
2023-02-20 14:22     ` Thomas Zimmermann
2023-02-20 14:22       ` Thomas Zimmermann
2023-02-20 15:39       ` Gerd Hoffmann
2023-02-20 15:39         ` Gerd Hoffmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y+5l+BbN7JjpaQlH@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=airlied@redhat.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=javierm@redhat.com \
    --cc=sam@ravnborg.org \
    --cc=tzimmermann@suse.de \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.